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

move augmentClass definition above SyntheticEvent Proxy replacement #10011

Merged
merged 1 commit into from Jul 11, 2017

Conversation

Projects
None yet
4 participants
@walrusfruitcake
Contributor

walrusfruitcake commented Jun 20, 2017

Before submitting a pull request, please make sure the following is done:

  1. Fork the repository and create your branch from master.
  2. If you've added code that should be tested, add tests!
  3. If you've changed APIs, update the documentation.
  4. Ensure the test suite passes (npm test).
  5. Make sure your code lints (npm run lint).
  6. Format your code with prettier (npm run prettier).
  7. Run the Flow typechecks (npm run flow).
  8. If you added or removed any tests, run ./scripts/fiber/record-tests before submitting the pull request, and commit the resulting changes.
  9. If you haven't already, complete the CLA.
@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jun 20, 2017

Member

The browser mentioned in #10010 is QTWebkit, which appears to be an unofficial fork of Webkit. While this change is trivial, I'm hesitant to include changes meant to address dev-only issues in unsupported browsers.

The usage of Proxy here appears to be correct, which indicates that the Proxy implementation GTWebkit is using is possibly incorrect (especially since this Proxy call has been working in Webkit browsers for a long time).

Member

aweary commented Jun 20, 2017

The browser mentioned in #10010 is QTWebkit, which appears to be an unofficial fork of Webkit. While this change is trivial, I'm hesitant to include changes meant to address dev-only issues in unsupported browsers.

The usage of Proxy here appears to be correct, which indicates that the Proxy implementation GTWebkit is using is possibly incorrect (especially since this Proxy call has been working in Webkit browsers for a long time).

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 20, 2017

Member

We could get it in but there's no guarantee we won't break again.

Member

gaearon commented Jun 20, 2017

We could get it in but there's no guarantee we won't break again.

postgres-mirror pushed a commit to postgres/pgadmin4 that referenced this pull request Jun 21, 2017

Fix React to work with QtWebKit
We learned that the underlying issue was related to react-dom's SyntheticEvent.augmentClass function being undefined.

This seems to be caused by attempted property assignment after the SyntheticEvent had been replaced by a Proxy of itself. This works fine in Chromium et al, but QtWebKit doesn't deal with Proxy Event objects well.
Moving the augmentClass definition and assignment up above the Proxy stuff resolves the issue in a PR to React: facebook/react#10011
@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jul 5, 2017

Member

@gaearon do you want to make the call here? It's a (seemingly) innocent change, but I'm hesitant to accept since it sets a precedence for including changes for unsupported environments. But it's up to you.

Member

aweary commented Jul 5, 2017

@gaearon do you want to make the call here? It's a (seemingly) innocent change, but I'm hesitant to accept since it sets a precedence for including changes for unsupported environments. But it's up to you.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 6, 2017

Member

If it doesn't break anything I'm cool with taking it.

Member

gaearon commented Jul 6, 2017

If it doesn't break anything I'm cool with taking it.

@aweary aweary self-assigned this Jul 6, 2017

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jul 6, 2017

Member

Cool, I'll verify everything looks OK locally and merge if so

Member

aweary commented Jul 6, 2017

Cool, I'll verify everything looks OK locally and merge if so

@gaearon gaearon merged commit b79619e into facebook:master Jul 11, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 11, 2017

Member

Seems fine to me, I'll get it in.

Member

gaearon commented Jul 11, 2017

Seems fine to me, I'll get it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment