WIP Fix react dom factories #9793
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Also - this is WIP because we haven't actually published the module to npm, so the 'require' in |
sophiebits
requested changes
May 26, 2017
src/isomorphic/React.js
Outdated
| var ReactBaseClasses = require('ReactBaseClasses'); | ||
| var ReactChildren = require('ReactChildren'); | ||
| var ReactDOMFactories = require('ReactDOMFactories'); | ||
| var reactDOMFactories = require('react-dom-factories'); |
Collaborator
There was a problem hiding this comment.
sooo I'm concerned about this circular dependency (this imports react-dom-factories imports react which is this) -- I think it's okay if we put it in a getter that requires it asynchronously which we'd need anyway in order to warn for it.
**what is the change?:** First steps towards making this package something we can publish on 'npm' and use in a variety of contexts. **why make this change?:** We need to get this published to unblock the 15.6 React release. **test plan:** None yet. **issue:** facebook#9398
**what is the change?:** Adds an Immediately Invoked Function Expression around the body of the 'react-dom-factories' module, such that it will make the module available via 'require', or via AMD, or via the global scope, depending on what is supported. **why make this change?:** Trying to find the simplest way possible to make this ready for `npm publish`. **test plan:** WIP **issue:** facebook#9398
**what is the change?:**
Moves `reauire('react-dom-factories')` to separate 'getter' module.
**why make this change?:**
To avoid circular dependency.
**test plan:**
WIP
flarnie
force-pushed
the
fixReactDomFactories
branch
from
c25e3a5 to
6efb304
Compare
Contributor
Author
|
Made a new cleaner PR addressing this - #9823 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to publish the
react-dom-factoriespackage we need to make it work with AMD, global scope, and all other builds.Since this should only be published once, hopefully, my initial approach is the simplest possible and doesn't automate anything.
Next steps are to test this but I'd be happy for feedback and tips.