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

Add react-dom-unstable-native-dependencies #10138

Merged
merged 6 commits into from
Jul 12, 2017
Merged

Conversation

fmoo
Copy link
Contributor

@fmoo fmoo commented Jul 11, 2017

react-native-web and react-primitives currently access a few internals
for shimming DOM events into native ones. Changes in react@16 packaging
hide these internals, which has made it difficult for these projects to upgrade.
change adds a submodule to react-dom, unstable-native-dependencies that
includes the necessary modules to continue enabling that method of
dom-native event injection.

The bundle config is currently a bit of dogscience. The one thing I really changes were:

  • basically copied it from react-dom
  • added an externals / globals alias for react-dom in order to import it from inside the module
    which was required to inject the shared ComponentTree into this module.
  • removed the fb entry point since I wasn't sure what it did or if it would be necessary

react-native-web and react-primitives currently access a few internals
for shimming DOM events into native ones.  Changes in react@16 packaging
hide these internals completely.  This change adds a submodule to react-dom,
unstable-native-dependencies that includes the necessary modules to
continue enabling that method of dom-native event injection.
@gaearon
Copy link
Collaborator

gaearon commented Jul 11, 2017

removed the fb entry point since I wasn't sure what it did or if it would be necessary

It would be useful if we wanted to use these modules in www too.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Small changes.

@@ -0,0 +1,7 @@
'use strict';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since there's a new file we need to add it to whitelist in package.json, or it would get ignored on publish.

@@ -125,6 +125,34 @@ const bundles = [
'src/shared/**/*.js',
],
},
/* React DOM internals required for shimming react-native apps into the react-dom renderer */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe let's say "required for react-native-web" for explicitness.

@gaearon
Copy link
Collaborator

gaearon commented Jul 11, 2017

Is there a smoke test we can add? Otherwise I'm worried we'll break it.

fmoo added 2 commits July 11, 2017 14:30
In order to get some sort of smoke testing on
react-dom-unstable-native-dependencies, update ResponderEventPlugin-test
to use the "public" interfaces provided by react-dom and the new
react-dom/unstable-native dependencies

Also adds the missing references in package.json as well as missing
files required for unittests to do imports correctrly

Also exports injectComponentTree() which is required for the unittests
to re-set the shared component state between runs.
@fmoo
Copy link
Contributor Author

fmoo commented Jul 11, 2017

Still trying to figure out the fbEntry business. When I add it and try to build, I get the following unhandled error when trying to build:

'ReactDOM' is imported by src/renderers/dom/shared/ReactDOMUnstableNativeDependenciesEntry.js,
but could not be resolved – treating it as an external dependency

Any ideas?

@gaearon
Copy link
Collaborator

gaearon commented Jul 12, 2017

Please yarn prettier, thanks!

@gaearon gaearon merged commit cff012f into facebook:master Jul 12, 2017
@gaearon
Copy link
Collaborator

gaearon commented Jul 12, 2017

LGTM, thank you for driving this!

@necolas
Copy link
Contributor

necolas commented Jul 12, 2017

I really appreciate this. Thank you!

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

Successfully merging this pull request may close these issues.

4 participants