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

Prepare react-dom-factories for publishing #9823

Merged
merged 2 commits into from
May 31, 2017

Conversation

flarnie
Copy link
Contributor

@flarnie flarnie commented May 31, 2017

what is the change?:

  • copies the React.DOM.* factory helpers into the package and wraps
    them with an IIFE
  • set peerDependency on React to latest 15 version

why make this change?:
We are deprecating React.DOM.* and this provides a fallback option for
those using it.

We just copied the code in order to avoid a circular dependency, which
keeps complexity down since we are supporting every possible build
system.

test plan:

DONE -

  1. Copy this into one of our fixtures using a UMD/AMD build and see if
    it works there.

TODO -
2. Do an initial publish of the package on npm and then pull it into
CRA, and try it there.
3. Try it in https://github.com/duncanbeevers/jade-react or some other
project that actually uses the React.DOM.* helpers, using the
related codemod.

issue:
#9398

**what is the change?:**
- copies the `React.DOM.*` factory helpers into the package and wraps
  them with an IIFE
- set peerDependency on React to latest 15 version

**why make this change?:**
We are deprecating `React.DOM.*` and this provides a fallback option for
those using it.

We just copied the code in order to avoid a circular dependency, which
keeps complexity down since we are supporting every possible build
system.

**test plan:**
TODO -
1. Copy this into one of our fixtures using a UMD/AMD build and see if
   it works there.
2. Do an initial publish of the package on npm and then pull it into
   CRA, and try it there.
3. Try it in https://github.com/duncanbeevers/jade-react or some other
   project that actually uses the `React.DOM.*` helpers, using the
   related codemod.

**issue:**
facebook#9398
Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

lgtmacro

}

if (typeof g.React === "undefined") {
throw Error("React module should be required before reactDOMFactories");
Copy link
Collaborator

Choose a reason for hiding this comment

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

ReactDOMFactories

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

Nothing else beyond what Ben said. Looks good to me!

**what is the change?:**
Fixing nits in capitalization, quotes, and defined globals

**why make this change?:**
To get CI passing, consistency, correctness

**test plan:**
see prev. commit

**issue:**
facebook#9398
@flarnie
Copy link
Contributor Author

flarnie commented May 31, 2017

Testing of the UMD build in our 'systemjs', 'requirejs', and 'globals' fixtures was successful.
screen shot 2017-05-31 at 3 46 36 pm

@flarnie flarnie merged commit 78fc25f into facebook:15.6-dev May 31, 2017
@flarnie
Copy link
Contributor Author

flarnie commented Jun 1, 2017

Further testing of the npm module with all other builds showed no problems.
screen shot 2017-05-31 at 3 46 36 pm

flarnie added a commit to flarnie/react that referenced this pull request Jun 7, 2017
* Prepare `react-dom-factories` for publishing

**what is the change?:**
- copies the `React.DOM.*` factory helpers into the package and wraps
  them with an IIFE
- set peerDependency on React to latest 15 version

**why make this change?:**
We are deprecating `React.DOM.*` and this provides a fallback option for
those using it.

We just copied the code in order to avoid a circular dependency, which
keeps complexity down since we are supporting every possible build
system.

**test plan:**
TODO -
1. Copy this into one of our fixtures using a UMD/AMD build and see if
   it works there.
2. Do an initial publish of the package on npm and then pull it into
   CRA, and try it there.
3. Try it in https://github.com/duncanbeevers/jade-react or some other
   project that actually uses the `React.DOM.*` helpers, using the
   related codemod.

**issue:**
facebook#9398

* Fix lints and capitalization of 'reactDOMFactories'

**what is the change?:**
Fixing nits in capitalization, quotes, and defined globals

**why make this change?:**
To get CI passing, consistency, correctness

**test plan:**
see prev. commit

**issue:**
facebook#9398
@flarnie flarnie deleted the fixReactDomFactories2 branch May 25, 2018 17:51
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