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.

Sign up for GitHub

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

Jump to bottom

Call getComponent with nextState instead of location #3298

Merged
merged 1 commit into from
Apr 13, 2016
Merged

Call getComponent with nextState instead of location #3298

merged 1 commit into from
Apr 13, 2016

Conversation

taion
Copy link
Contributor

@taion taion commented Apr 13, 2016

Fixes #3262

@taion
Copy link
Contributor Author

taion commented Apr 13, 2016

From taion/rrtr#16

@taion taion force-pushed the getComponent-nextState branch 2 times, most recently from 18da3c9 to 41cd4c2 Compare April 13, 2016 19:52
@taion taion mentioned this pull request Apr 13, 2016
Closed
timdorr
timdorr reviewed Apr 13, 2016
View reviewed changes
modules/getComponents.js

if (__DEV__) {
if (typeof Proxy === 'function') {
nextStateWithLocation = new Proxy(nextStateWithLocation, {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this without Proxy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use something like deprecateObjectProperties, but it'd be a bit more code. This is what React does upstream to prevent people from messing with synthetic events: https://github.com/facebook/react/blob/v15.0.1/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js#L188.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, no, I guess they do something different for properties: https://github.com/facebook/react/blob/v15.0.1/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js#L167. I'll update this.

@taion
Copy link
Contributor Author

taion commented Apr 13, 2016

I've updated the PR to use Object.defineProperty (when possible) to assign deprecated properties to nextStateWithLocation, rather than using a Proxy.

@taion
Copy link
Contributor Author

taion commented Apr 13, 2016

I'd like to cut v2.2.0 after we merge this, if possible. (I'll update the changelog)

@ryanflorence
Copy link
Member

👍

@ryanflorence ryanflorence merged commit 0cdee03 into remix-run:master Apr 13, 2016
@taion taion deleted the getComponent-nextState branch April 13, 2016 23:27
@renovate renovate bot mentioned this pull request Oct 11, 2018
Closed
1 task
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@taion @ryanflorence @timdorr