Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply component and mixins specs deterministically #1601

Merged
merged 1 commit into from Aug 16, 2014

Conversation

@gaearon
Copy link
Member

gaearon commented May 25, 2014

Fixes #1589.

@gaearon

This comment has been minimized.

Copy link
Member Author

gaearon commented May 25, 2014

@spicyj

Here's my first take on it. I'm not sure if it's better to leave mixins defined inside RESERVED_SPEC_KEYS or to separate them to emphasize that it is a special case.

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.

@gaearon

This comment has been minimized.

Copy link
Member Author

gaearon commented May 25, 2014

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?

@sophiebits
sophiebits reviewed May 25, 2014
View changes
src/core/ReactCompositeComponent.js Outdated
// 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')) {

This comment has been minimized.

Copy link
@sophiebits

sophiebits May 25, 2014

Collaborator

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.

@sophiebits
sophiebits reviewed May 25, 2014
View changes
src/core/__tests__/ReactCompositeComponentMixin-test.js Outdated
@@ -128,6 +155,19 @@ describe('ReactCompositeComponent-mixin', function() {
]);
});

it('should chain delegate functions regardless of property order in spec', function() {

This comment has been minimized.

Copy link
@sophiebits

sophiebits May 25, 2014

Collaborator

This is over 80 chars. If you do it('chains ... then you save a bit but you might still need to be creative to squish it a bit more.

@sophiebits
sophiebits reviewed May 25, 2014
View changes
src/core/__tests__/ReactCompositeComponentMixin-test.js Outdated
componentDidMount: function() {
this.props.listener('Component didMount');
},
propTypes: {

This comment has been minimized.

Copy link
@sophiebits

sophiebits May 25, 2014

Collaborator

Not sure you need propTypes or statics here?

@sophiebits

This comment has been minimized.

Copy link
Collaborator

sophiebits commented May 25, 2014

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.

@gaearon

This comment has been minimized.

Copy link
Member Author

gaearon commented May 25, 2014

Oops, one moment

@gaearon

This comment has been minimized.

Copy link
Member Author

gaearon commented May 25, 2014

There you go.

@sophiebits
sophiebits reviewed May 25, 2014
View changes
src/core/ReactCompositeComponent.js Outdated
// 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]);

This comment has been minimized.

Copy link
@sophiebits

sophiebits May 25, 2014

Collaborator

(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.)

@sophiebits
sophiebits reviewed May 25, 2014
View changes
src/core/ReactCompositeComponent.js Outdated
// 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(ConvenienceConstructor, spec[MIXINS_KEY]);

This comment has been minimized.

Copy link
@sophiebits

sophiebits May 25, 2014

Collaborator

(spec.mixins too!)

@gaearon

This comment has been minimized.

Copy link
Member Author

gaearon commented May 25, 2014

I finally got it. :-) If I need it as a string, I use keyOf. Otherwise, I access property via dot and don't think about it.

@sophiebits

This comment has been minimized.

Copy link
Collaborator

sophiebits commented May 25, 2014

Yup! This looks good to me now. 👍

@gaearon

This comment has been minimized.

Copy link
Member Author

gaearon commented May 25, 2014

Thanks for quick response. 🏇

@zpao

This comment has been minimized.

Copy link
Member

zpao commented May 28, 2014

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 :)

@zpao

This comment has been minimized.

Copy link
Member

zpao commented Jun 24, 2014

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 :)

@yungsters

This comment has been minimized.

Copy link
Contributor

yungsters commented Jun 24, 2014

Yay! This l

@yungsters

This comment has been minimized.

Copy link
Contributor

yungsters commented Jun 24, 2014

Dang it, GitHub on mobile is hard.

Yay! Looks good. Thanks for fixing this.

@gaearon

This comment has been minimized.

Copy link
Member Author

gaearon commented Jun 24, 2014

@zpao No problem, ping me.

@zpao

This comment has been minimized.

Copy link
Member

zpao commented Aug 13, 2014

@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…

@zpao

This comment has been minimized.

Copy link
Member

zpao commented Aug 13, 2014

cc @leebyron too :)

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Aug 15, 2014

This is good. Let's get this in.

@leebyron

This comment has been minimized.

Copy link
Contributor

leebyron commented Aug 16, 2014

LGTM

leebyron added a commit that referenced this pull request Aug 16, 2014
…deterministically

Apply component and mixins specs deterministically
@leebyron leebyron merged commit 04c9820 into facebook:master Aug 16, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@chenglou

This comment has been minimized.

Copy link
Contributor

chenglou commented Aug 16, 2014

@leebyron I think you merged this incorrectly. https://github.com/facebook/react/pull/1601/files#diff-2ee62d642502ef4bdc3a6b3d18cd4f99R464 specifically (should be Constructor). The internal version's fine. Not sure what happened here.

The if (!spec) code also didn't get checked in.

@zpao

This comment has been minimized.

Copy link
Member

zpao commented Aug 17, 2014

@chenglou feel free to fix 😉

@gaearon gaearon deleted the gaearon:apply-component-and-mixin-spec-deterministically branch Jan 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.