Apply component and mixins specs deterministically #1601
Conversation
|
Here's my first take on it. I'm not sure if it's better to leave I think we should now clearly state the order in the docs. This is something I had to look up in source code when I first faced it. |
|
Oh and by the way: I have a problem with the linter, for some reason it outputs a lot of warnings unrelated to the code I changed. Is this normal? |
| // By handling mixins before any other properties, we ensure the same | ||
| // chaining order is applied to methods with DEFINE_MANY policy, whether | ||
| // mixins are listed before or after these methods in the spec. | ||
| if (spec.hasOwnProperty('mixins')) { |
There was a problem hiding this comment.
We try to be compatible with Google Closure Compiler advanced mode, which crushes object keys. (So spec.mixins becomes spec.zb or something like that.) In order to make sure that the key we're checking here is the same as the crushed version, you should do
var MIXINS_KEY = keyOf({mixins: null});
at the top of the file and then use MIXINS_KEY here instead of 'mixins' directly.
|
I would have said it's clearer to take it out of RESERVED_SPEC_KEYS but then you still need to special-case skipping it in the loop, so I think it's fine as you have it. Yeah, unfortunately lint is a little too picky on some things and doesn't pass right now. |
|
Oops, one moment |
|
There you go. |
| // chaining order is applied to methods with DEFINE_MANY policy, whether | ||
| // mixins are listed before or after these methods in the spec. | ||
| if (spec.hasOwnProperty(MIXINS_KEY)) { | ||
| RESERVED_SPEC_KEYS[MIXINS_KEY](ConvenienceConstructor, spec[MIXINS_KEY]); |
There was a problem hiding this comment.
(On this line it's fine to write .mixins because it'll get crushed -- it's only when using it as a string explicitly that you need to use the constant.)
|
I finally got it. :-) If I need it as a string, I use |
|
Yup! This looks good to me now. 👍 |
|
Thanks for quick response. 🏇 |
|
I'm not going to be around to bring this in and a bunch of the team is at JSConf or on vacation so we might not get this in for a couple days. I think it looks good though, thanks for jumping on it :) |
|
cc @yungsters. This got done the way you wanted :) @gaearon, we have a change around some of this code from @sebmarkbage that I'll sync out ASAP. So there will be a little bit of rebase work coming. Hope you're still up for it :) |
|
Yay! This l |
|
Dang it, GitHub on mobile is hard. Yay! Looks good. Thanks for fixing this. |
|
@zpao No problem, ping me. |
|
@sebmarkbage can you take a look at this? It would be good to have deterministic behavior but I know there's still a bunch of changes happening around component creation… |
|
cc @leebyron too :) |
|
This is good. Let's get this in. |
|
LGTM |
…deterministically Apply component and mixins specs deterministically
|
@leebyron I think you merged this incorrectly. https://github.com/facebook/react/pull/1601/files#diff-2ee62d642502ef4bdc3a6b3d18cd4f99R464 specifically (should be The |
|
@chenglou feel free to fix 😉 |
Fixes #1589.