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

Rewrite for React Hot Loader 3 #56

Merged
merged 6 commits into from
Mar 6, 2017
Merged

Rewrite for React Hot Loader 3 #56

merged 6 commits into from
Mar 6, 2017

Conversation

gaearon
Copy link
Owner

@gaearon gaearon commented Apr 17, 2016

Changes related to the “wrap during createElement” approach described here:

  • The current class is now this.constructor, not the proxy class
  • The passed classes are mutated, e.g. when you update(), static properties created dynamically on the previous class get copied to the next class
  • Tests are now properly isolated because of this

Example of this in action: gaearon/react-hot-boilerplate#61

try {
// Create a proxy constructor with matching name
ProxyComponent = new Function('factory', 'instantiate',
`return function ${InitialComponent.name || 'ProxyComponent'}() {
`return function ${displayName}() {
Copy link

@leidegre leidegre Apr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon this appears to cause an issue with Redux, it tries to evaluate Connect(ComponentName) as a function name and it results in a silent SyntaxError. It only shows up if you run with Pause On Caught Exceptions and no other error in console. Is it necessary to sanitize the function name?

There is a reasonable fallback here. No further action might be necessary.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, we should sanitize it. Would you like to send a PR against 3.x branch?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do what I can.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made an attempt here #58

@ghost
Copy link

ghost commented Apr 25, 2016

The current class is now this.constructor, not the proxy class

I think this will actually make it easier for me to use a Proxy here because this is where I ended up spinning my wheels last time

@gaearon
Copy link
Owner Author

gaearon commented Apr 25, 2016

Oh, cool, would be excited to see that @danmartinez101 😄

@glenjamin
Copy link

How would you feel about exposing a lifecycle hook for when a proxy is updated?

I'm trying to find a nice way of having methods rebind to the instance when using ES6 Classes.

@wkwiatek wkwiatek merged commit c679ad8 into master Mar 6, 2017
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.

4 participants