[Bug] CSSPropertyOperations.setValueForStyles does not check for `undefined` component argument

[pke]

Version: 15.1.0

setValueForStyles: function(node, styles, component) {
    if (__DEV__) {
      ReactInstrumentation.debugTool.onHostOperation(
        component._debugID,
        'update styles',
        styles
      );
    }

react/src/renderers/dom/shared/CSSPropertyOperations.js

Line 195 in ba9b985

if (__DEV__) {

All other code in that component is tolerant about a missing component argument.
This changed introduced a breaking bug in https://github.com/adambbecker/react-style-transition-group and the way it calls CSSPropertyOperations.setValueForStyles without a component.

[jimfb]

CSSPropertyOperations.setValueForStyles is not public API and should not be used by third-party code. Feel free to file a bug on the third-party repository.

[pke]

I see. I filed a bug on the third-party code but the issue is not resolvable except with a hack to trick react with a fake component.
Even though it is an internal API the usage of the component inside this internal API is not consistent. All other code in the same file do a proper component check.

Since this particular code is only used in __DEV__ code paths anyway, what stops you put a simple null check in the code as in the rest of the file?

[gaearon]

All other code in the same file do a proper component check.

Do you mean this single line? I don’t see any other places where component is used. (Not trying to be nitpicky, just want to make sure I understand you.)

The reason why I don’t think it makes sense to check for null is that the only line that calls this method always supplies component argument. How can it ever be null? Perhaps it was possible in the past, but it doesn’t seem now so I don’t think there’s anything that needs fixing in React.

[pke]

Yep, this line :) I understand the issue and will have to migrate to another page transition module then.
Maybe a good thing after all.

[buildbreakdo]

@gaearon @jimfb

To clarify: believe he means none of the CSSPropertyOperations functions (__DEV__ warnings included) or imported modules previously had component as a required parameter (believe for a good reason). Commit @pke referenced is a divergence from the pattern of __DEV__ only params being optional (granted the imported modules follow the same pattern, assuming this is a general pattern in React at large [correct if wrong here]). @edvinerikson, see you added component name to CSS warnings wrapped in owner checks here, what are your thoughts? Is it a general practice for __DEV__ only parameters to be optional?

Think this also hints at a bigger architectural discussion that needs to be had, mainly answering the questions:

• Should __DEV__ only sugar--like helpful warnings--be permitted to lessen the sharability of a shared module? (via required parameters narrowing the contexts in which a module can be used; is this also used in react native style? If not shouldn't it be?)
• Should __DEV__ only parameters ever be required or always be optional? Overtime if they are required, do believe it increases rigidity and bloat of modules and the framework at large. Module rigidity should not be a function of dev tooling.
• In light of the precedent started before (null checks in CSSPropertyOperations and most importantly in imported modules[means this pattern is not a one off?]); __DEV__ only parameters were optional, so the answer was no, dev only params should not be required; is it now yes?

Either way, wanted to make sure that this departure from a norm (at least--if not only--in this module and those imported) was a conscious architecture decision being made for React, do believe a pivot down this path has significant implications for shared resources in regards to.. well... their shareability! Also.. kind of smelly (breaks in dev without component but not in prod)! :)

Tagging @sebmarkbage the refactoring guy, granted he put (or was instructed to put) this module in the shared folder and may have a marked (pun intended) interest in how shared resources evolve. Cheers.

[pke]

Also.. kind of smelly (breaks in dev without component but not in prod)! :)

That's exactly my point. It either breaks always or never. Since the component is only used for some debug helping output it should be optional.

[jimfb]

@pke @buildbreakdo I feel like there is an impedance mismatch; you and @gaearon are talking right past each other. In this case, @gaearon is correct.

Think this also hints at a bigger architectural discussion that needs to be had, mainly answering the questions...

No, there is no larger architectural discussion. The questions are irrelevant because the function works for React's use case (that parameter is always available). The function works as intended, and the set of required parameters has never caused a problem within the core, so it's just simply not a problem. The function is NOT public API.

[gaearon]

Should DEV only sugar--like helpful warnings--be permitted to lessen the sharability of a shared module?

It is only "shared" between DOM client and DOM server renderer code. It is not "shared" in the sense that it can be used elsewhere, and there is no further desire for to sharability. DOM client and DOM server renderers are intended to be its only users.

There is no bigger picture here, I think. The method signature can change every day, and we may add or remove parameters, whether or not they are DEV-only. Please don't rely on this method because it is not public API.