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

(WIP) Get rid of _source #272

Merged
merged 14 commits into from
May 2, 2016
Merged

(WIP) Get rid of _source #272

merged 14 commits into from
May 2, 2016

Conversation

gaearon
Copy link
Owner

@gaearon gaearon commented May 1, 2016

Should fix #266 and #249.

TODO:

  • Fix Webpack loader as well
  • Only rely on WeakMap if it's available
  • Use global npm package instead of window
  • Revive the warning about calling ReactDOM.render() in the same file
  • Test edge cases
  • Test React Router support (manually)

@nfcampos
Copy link
Collaborator

nfcampos commented May 1, 2016

You might want to test this with the kind of tests I have in #269 replacing the call to tag with the set on the global map

`separate file for hot reloading to work.`
);
}
var id = idsByType.get(type);
Copy link
Collaborator

@nfcampos nfcampos May 1, 2016

Choose a reason for hiding this comment

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

I think this will update the proxy even if createElement is called with stale type only if we additionally keep a typesById map and do var latestType = typesById.get(id) after this line and use this latestType to update the proxy. Otherwise it doesn't solve the router problem

Copy link
Owner Author

Choose a reason for hiding this comment

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

The reason stale types were used is because Router cached old types and called createElement with them. Now the only “entry point” to updating a proxy is an explicit __REACT_HOT_LOADER__ call. So router can’t revive old types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point.

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.

2 participants