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

Generalize copying logic by using descriptors #37

Merged
merged 1 commit into from
Aug 17, 2017

Conversation

jamesreggio
Copy link
Contributor

This PR is a revival of #28.

First, it does something simple, which is to use defineProperty instead of an equality-based assignment. This is better JS hygiene when copying properties, since equality assignment results in the loss of metadata and can sometimes trigger side-effects.

Second, it removes the enumerable-or-function conditional. Judging by how the 'or-function' was added to this condition only 18 days ago (65f3d58), I think we can reasonably agree that this condition is arbitrary. As discussed in my earlier PR (#28), both static functions and complex properties (those with getter/setter methods) are non-enumerable, and both are desirable for hoisting. However, complex properties cannot be 'detected' with a typeof operator.

In the end, if we have a complete blacklist (as we do), there should be no harm in copying all non-blacklisted static properties from one component to another, so that's what I'm proposing here.

@mridgway
Copy link
Owner

There was desire to only hoist enumerable properties, which was quickly found to not be effective in the case of static functions. As you point out, there are probably other cases that make the original desire less ideal.

At this point I am in favor of this change, but need to think about whether this could break existing users in some way.

@jamesreggio
Copy link
Contributor Author

Thanks for considering. I've been wracking my brain for examples of static properties that should not be copies over, but have come up emptyhanded. I'll keep thinking and let you know if anything comes to mind.

@jamesreggio
Copy link
Contributor Author

Hi @mridgway, have you thought of any scenarios in which these changes will break existing users?

@mridgway
Copy link
Owner

I have not. I will move forward with merging this.

@mridgway mridgway merged commit 0391f6f into mridgway:master Aug 17, 2017
@mridgway
Copy link
Owner

It does look like defineProperty will need to be polyfilled in IE8 in order to work correctly, so I will release this as a major version bump with a note on this.

@jamesreggio
Copy link
Contributor Author

Great, thanks!

@jamesreggio jamesreggio deleted the support-complex-properties-2 branch August 17, 2017 22:13
@mridgway
Copy link
Owner

Looks like React is using defineProperty internally as well, so I'm comfortable with this being a minor. v2.3.0 coming soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants