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

Set 'React' as dependency in AMD builds of addons #9765

Closed
flarnie opened this issue May 24, 2017 · 1 comment
Closed

Set 'React' as dependency in AMD builds of addons #9765

flarnie opened this issue May 24, 2017 · 1 comment
Assignees
Milestone

Comments

@flarnie
Copy link
Contributor

flarnie commented May 24, 2017

Right now 'React' is undefined in some of our addons which may depend on it:

The problem: running these in an AMD environment without React defined globally will lead to the dependency not being found.

Probably this can be solved as follows:

  1. Manually edit the unminified version, making it work like react-dom:
    18697850_420365161667842_787782393_o
  2. Test by modify requirejs or systemjs fixtures as one-offs to test it specifically. This doesn't need to be committed.
  3. Submit a PR and a React Core maintainer will rebuild the minified version for you - this is not a process we had intended to repeat so it is not automated in an easy way to reproduce. tag @flarnie and/or @gaearon

Ideally we are not making changes to the addons code, but this is worth doing. If someone wants to work on this I'm happy to review PRs, otherwise will pick it up in the next couple of days.

@flarnie flarnie added this to the 15.6 milestone May 24, 2017
@flarnie flarnie self-assigned this May 24, 2017
@flarnie flarnie mentioned this issue May 24, 2017
49 tasks
flarnie added a commit to flarnie/react that referenced this issue Jun 9, 2017
NOTE: Never going to merge this commit, but I may cherry-pick it onto
branches in order to test fixes for issue facebook#9765

**what is the change?:**
Require and use the UMD bundles of 'create-react-class' in three
fixtures to test the three supported uses;
 - test Global JS with globals.html
 - test AMD with requirejs.html
 - test CommonJS with webpack-alias

**why make this change?:**
To test facebook#9761 and other PRs fixing facebook#9765

**test plan:**
Manual testing;
 - cd into the directory in fixtures
 - run the build step if needed
 - open the file

**issue:**
facebook#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 9, 2017
NOTE: Never going to merge this commit, but I may cherry-pick it onto
branches in order to test fixes for issue facebook#9765

**what is the change?:**
Require and use the UMD bundles of 'create-react-class' in three
fixtures to test the three supported uses;
 - test Global JS with globals.html
 - test AMD with requirejs.html
 - test CommonJS with webpack-alias

**why make this change?:**
To test facebook#9761 and other PRs fixing facebook#9765

**test plan:**
Manual testing;
 - cd into the directory in fixtures
 - run the build step if needed
 - open the file

**issue:**
facebook#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 10, 2017
NOTE: Never going to merge this commit, but I may cherry-pick it onto
branches in order to test fixes for issue facebook#9765

**what is the change?:**
Require and use the UMD bundles of 'create-react-class' in three
fixtures to test the three supported uses;
 - test Global JS with globals.html
 - test AMD with requirejs.html
 - test CommonJS with webpack-alias

**why make this change?:**
To test facebook#9761 and other PRs fixing facebook#9765

**test plan:**
Manual testing;
 - cd into the directory in fixtures
 - run the build step if needed
 - open the file

**issue:**
facebook#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 10, 2017
NOTE: Never going to merge this commit, but I may cherry-pick it onto
branches in order to test fixes for issue facebook#9765

**what is the change?:**
Require and use the UMD bundles of 'create-react-class' in three
fixtures to test the three supported uses;
 - test Global JS with globals.html
 - test AMD with requirejs.html
 - test CommonJS with webpack-alias

**why make this change?:**
To test facebook#9761 and other PRs fixing facebook#9765

**test plan:**
Manual testing;
 - cd into the directory in fixtures
 - run the build step if needed
 - open the file

**issue:**
facebook#9765
flarnie added a commit to mondwan/react that referenced this issue Jun 10, 2017
NOTE: Never going to merge this commit, but I may cherry-pick it onto
branches in order to test fixes for issue facebook#9765

In this case I will clean it up afterwards.

**what is the change?:**
Require and use the UMD bundles of 'create-react-class' in three
fixtures to test the three supported uses;
 - test Global JS with globals.html
 - test AMD with requirejs.html
 - test CommonJS with webpack-alias

**why make this change?:**
To test facebook#9761 and other PRs fixing facebook#9765

**test plan:**
Manual testing;
 - cd into the directory in fixtures
 - run the build step if needed
 - open the file

**issue:**
facebook#9765
flarnie added a commit to mondwan/react that referenced this issue Jun 10, 2017
**what is the change?:**
Pass the global 'react' into the global conditional in the UMD build of
'create-react-class'.

**why make this change?:**
Here is the deal:
 - @mondwan's original fix does fix the AMD build, but breaks the
   'global JS' build.
 - My modification makes it work with both AMD and the 'global JS'
   build.
 - @mondwan's fix seems to have fixed the CommonJS build too, and I
   maintained that fix with my modification.

```
                Does the 'create-react-class' UMD build work?

                 Before       After         After
               + @mondwan's + @mondwan's +  @flarnie's
  Build System | fix        | fix        |  modification
+---------------------------------------------------------+
               |            |            |
  Global JS    | :D Success | X Fail     | :D Success
               |            |            |
+---------------------------------------------------------+
               |            |            |
  AMD          | X Fail     | :D Success | :D Success
               |            |            |
+---------------------------------------------------------+
               |            |            |
  Common JS    | X Fail     | :D Success | :D Success
               |            |            |
               +            +            +

```

**test plan:**
The testing for this was really tricky and involves a fragile multi-step
process:

1) Make sure the fixtures are working on your branch

2) Modify some of the fixtures to use 'create-react-class', like in this
   commit (you can just cherry-pick it if you are on a branch based on
   the 15.* branches) -
   flarnie@51dcbd5

3) Make sure React is set up, and then
   `cd fixures && node ./build-all.js`

4) The following fixtures could be used to test the various builds:
 - test GlobalJS with `globals.html`
 - test AMD with `requirejs.html`
 - test CommonJS with `webpack-alias/index.html`

**issue:**
facebook#9689
and
facebook#9765
@flarnie
Copy link
Contributor Author

flarnie commented Jun 10, 2017

I'm going to work on this, hopefully will get it resolved in time for the 15.6 release.

flarnie pushed a commit that referenced this issue Jun 10, 2017
* Fix missing react in create-react-class

refs #9689

* Test 'create-react-class' with fixtures

NOTE: Never going to merge this commit, but I may cherry-pick it onto
branches in order to test fixes for issue #9765

In this case I will clean it up afterwards.

**what is the change?:**
Require and use the UMD bundles of 'create-react-class' in three
fixtures to test the three supported uses;
 - test Global JS with globals.html
 - test AMD with requirejs.html
 - test CommonJS with webpack-alias

**why make this change?:**
To test #9761 and other PRs fixing #9765

**test plan:**
Manual testing;
 - cd into the directory in fixtures
 - run the build step if needed
 - open the file

**issue:**
#9765

* Remove fiber specific fixures

This already was merged (#9902)
but I wanted to do manual testing and needed the change locally.

**what is the change?:**
Remove 'fiber-debugger', 'fiber-triangle', and 'packaging' from
'fixtures' directory.

**why make this change?:**
These were not meant to be included on this branch and cause the
'build-all.js' script to throw.

**test plan:**
`cd ./fixtures && node ./build-all.js`

* Modify the 'create-react-class' package to make 'globals' work again

**what is the change?:**
Pass the global 'react' into the global conditional in the UMD build of
'create-react-class'.

**why make this change?:**
Here is the deal:
 - @mondwan's original fix does fix the AMD build, but breaks the
   'global JS' build.
 - My modification makes it work with both AMD and the 'global JS'
   build.
 - @mondwan's fix seems to have fixed the CommonJS build too, and I
   maintained that fix with my modification.

```
                Does the 'create-react-class' UMD build work?

                 Before       After         After
               + @mondwan's + @mondwan's +  @flarnie's
  Build System | fix        | fix        |  modification
+---------------------------------------------------------+
               |            |            |
  Global JS    | :D Success | X Fail     | :D Success
               |            |            |
+---------------------------------------------------------+
               |            |            |
  AMD          | X Fail     | :D Success | :D Success
               |            |            |
+---------------------------------------------------------+
               |            |            |
  Common JS    | X Fail     | :D Success | :D Success
               |            |            |
               +            +            +

```

**test plan:**
The testing for this was really tricky and involves a fragile multi-step
process:

1) Make sure the fixtures are working on your branch

2) Modify some of the fixtures to use 'create-react-class', like in this
   commit (you can just cherry-pick it if you are on a branch based on
   the 15.* branches) -
   flarnie@51dcbd5

3) Make sure React is set up, and then
   `cd fixures && node ./build-all.js`

4) The following fixtures could be used to test the various builds:
 - test GlobalJS with `globals.html`
 - test AMD with `requirejs.html`
 - test CommonJS with `webpack-alias/index.html`

**issue:**
#9689
and
#9765

* Undo modifications that add 'create-react-class' to fixtures

**what is the change?:**
In the previous commit we modified the fixtures to test
'create-react-class' manually, and this puts them all back.

**why make this change?:**
This will be useful for cherry-picking onto branches where we used the
previous commit for testing purposes

**test plan:**
`cd fixtures && node ./build-all.js` and open the fixtures

* remove stray console.log
flarnie pushed a commit that referenced this issue Jun 10, 2017
* Fix missing react in create-react-class

refs #9689

* Modify the 'create-react-class' package to make 'globals' work again

**what is the change?:**
Pass the global 'react' into the global conditional in the UMD build of
'create-react-class'.

**why make this change?:**
Here is the deal:
 - @mondwan's original fix does fix the AMD build, but breaks the
   'global JS' build.
 - My modification makes it work with both AMD and the 'global JS'
   build.
 - @mondwan's fix seems to have fixed the CommonJS build too, and I
   maintained that fix with my modification.

```
                Does the 'create-react-class' UMD build work?

                 Before       After         After
               + @mondwan's + @mondwan's +  @flarnie's
  Build System | fix        | fix        |  modification
+---------------------------------------------------------+
               |            |            |
  Global JS    | :D Success | X Fail     | :D Success
               |            |            |
+---------------------------------------------------------+
               |            |            |
  AMD          | X Fail     | :D Success | :D Success
               |            |            |
+---------------------------------------------------------+
               |            |            |
  Common JS    | X Fail     | :D Success | :D Success
               |            |            |
               +            +            +

```

**test plan:**
The testing for this was really tricky and involves a fragile multi-step
process:

1) Make sure the fixtures are working on your branch

2) Modify some of the fixtures to use 'create-react-class', like in this
   commit (you can just cherry-pick it if you are on a branch based on
   the 15.* branches) -
   flarnie@51dcbd5

3) Make sure React is set up, and then
   `cd fixures && node ./build-all.js`

4) The following fixtures could be used to test the various builds:
 - test GlobalJS with `globals.html`
 - test AMD with `requirejs.html`
 - test CommonJS with `webpack-alias/index.html`

**issue:**
#9689
and
#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 11, 2017
NOTE: Never going to merge this commit, but I may cherry-pick it onto
branches in order to test fixes for issue facebook#9765

**what is the change?:**
Require and use the UMD bundles of 'create-react-class' in three
fixtures to test the three supported uses;
 - test Global JS with globals.html
 - test AMD with requirejs.html
 - test CommonJS with webpack-alias

**why make this change?:**
To test facebook#9761 and other PRs fixing facebook#9765

**test plan:**
Manual testing;
 - cd into the directory in fixtures
 - run the build step if needed
 - open the file

**issue:**
facebook#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 11, 2017
**what is the change?:**
Renamed some fixtures.

**why make this change?:**
This is part of setting up manual tests of the add-ons we are fixing.

**test plan:**
`cd fixtures && node ./build-all.js` and manually open the renamed
fixtures.

**issue:**
facebook#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 11, 2017
**what is the change?:**
`prettier addons/react-linked-input/react-linked-input.js | pbcopy` and
replaced the contents of the file.

**why make this change?:**
I am manually tweaking this file and want it to be more readable.

**test plan:**
about to set up manual testing of this with fixtures. I expect that
right now only the use of it as a global will work, and subsequent
commits will fix the AMD and CommonJS use cases.

**issue:**
facebook#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 11, 2017
**what is the change?:**
Setting up the fixtures to enable manual testing of the
`react-linked-input` and `create-fragment` UMD builds.

This was a painstaking and frustrating process and we need something
better before making any more fixes to addons. Here is roughly what I
did;
- add 'console.log' statements to the add-on to confirm that you've loaded the right build case
- copy the add-on into 'build/packages' so that the 'webpack-alias' can find it.
- make copies of each of the following three fixtures for each add-on you want to test, renaming them to specify what you are testing:
	- globals.js
	- requirejs.js
	- webpack-alias/*
- modify those fixtures to use the add-on you intend to text

**why make this change?:**
We need to verify the current state of the bug before fixing it, to
confirm that the change actually is fixing the bug.

**test plan:**
`open fixtures/globals-with-create-react-fragment.html`
`open fixtures/globals-with-react-linked-input.html`
`open fixtures/requirejswith-create-react-fragment.html`
`open fixtures/requirejswith-react-linked-input.html`
`cd fixtures/webpack-aliaswith-create-react-fragment/ && yarn build && open index.html`
`cd fixtures/webpack-aliaswith-react-linked-input/ && yarn build && open index.html`

**issue:**
facebook#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 11, 2017
**what is the change?:**
Manually tweaking the UMD builds for both add-ons to include a
dependency on React.

**why make this change?:**
They were broken before for AMD and CommonJS.
For reasons I have not debugged, the CommonJS builds are still broken,
but the AMD is now fixed and globals still work:

```
    do 'react-linked-input' and
    'create-react-fragment' work?

                before      after
              + my        + my        +
  en^ironment | fix       | fix       |
+----------------------------------------
              |           |           |
  Global JS   |  :) yes   |  :) yes   |
+----------------------------------------
              |           |           |
  AMD         |  X no     |  :) yes   |
+----------------------------------------
              |           |           |
  CommonJS    |  X no     |  X no     |
+-------------+-----------+-----------+--

```

**test plan:**
In the previous commit we set up fixtures to manually test these.

**issue:**
facebook#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 11, 2017
**what is the change?:**
We forked three of the fixtures into two variations to test two of the
react addons. We also added `console.log` statements to the addons to
verify that we were loading the right build.

This commit cleans it up by
- deleting forked fixtures
- re-adding the original fixtures
- removing `console.log` statements

**why make this change?:**
To get this branch ready for review.

**test plan:**
`cd fixtures && node ./build-all.js` and then check the updated fixtures
manually

**issue:**
facebook#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 11, 2017
**what is the change?:**
We forked three of the fixtures into two variations to test two of the
react addons. We also added `console.log` statements to the addons to
verify that we were loading the right build.

This commit cleans it up by
- deleting forked fixtures
- re-adding the original fixtures
- removing `console.log` statements

**why make this change?:**
To get this branch ready for review.

**test plan:**
`cd fixtures && node ./build-all.js` and then check the updated fixtures
manually

**issue:**
facebook#9765
flarnie added a commit to flarnie/react that referenced this issue Jun 12, 2017
**what is the change?:**
`:%s /"/'/gc`

I left double quotes wrapping cases where we have single quotes in the
string.

**why make this change?:**
I ran the code for the unminified 'react-linked-input' through
'prettier' so it would be easier for me to manually fix the UMD wrapper.
And 'prettier' changed many single quotes into double quotes. @spicyj
pointed out this will be treated differently by the google closure
compiler, and may have semantic differences.

**test plan:**
It's not worth testing imo.

**issue:**
facebook#9765
flarnie added a commit that referenced this issue Jun 12, 2017
* Test 'create-react-class' with fixtures

NOTE: Never going to merge this commit, but I may cherry-pick it onto
branches in order to test fixes for issue #9765

**what is the change?:**
Require and use the UMD bundles of 'create-react-class' in three
fixtures to test the three supported uses;
 - test Global JS with globals.html
 - test AMD with requirejs.html
 - test CommonJS with webpack-alias

**why make this change?:**
To test #9761 and other PRs fixing #9765

**test plan:**
Manual testing;
 - cd into the directory in fixtures
 - run the build step if needed
 - open the file

**issue:**
#9765

* Rename fixtures testing create-react-class

**what is the change?:**
Renamed some fixtures.

**why make this change?:**
This is part of setting up manual tests of the add-ons we are fixing.

**test plan:**
`cd fixtures && node ./build-all.js` and manually open the renamed
fixtures.

**issue:**
#9765

* Prettify the unminified UMD build of `react-linked-input`

**what is the change?:**
`prettier addons/react-linked-input/react-linked-input.js | pbcopy` and
replaced the contents of the file.

**why make this change?:**
I am manually tweaking this file and want it to be more readable.

**test plan:**
about to set up manual testing of this with fixtures. I expect that
right now only the use of it as a global will work, and subsequent
commits will fix the AMD and CommonJS use cases.

**issue:**
#9765

* Test state of `react-linked-input` and `create-fragment` before fix

**what is the change?:**
Setting up the fixtures to enable manual testing of the
`react-linked-input` and `create-fragment` UMD builds.

This was a painstaking and frustrating process and we need something
better before making any more fixes to addons. Here is roughly what I
did;
- add 'console.log' statements to the add-on to confirm that you've loaded the right build case
- copy the add-on into 'build/packages' so that the 'webpack-alias' can find it.
- make copies of each of the following three fixtures for each add-on you want to test, renaming them to specify what you are testing:
	- globals.js
	- requirejs.js
	- webpack-alias/*
- modify those fixtures to use the add-on you intend to text

**why make this change?:**
We need to verify the current state of the bug before fixing it, to
confirm that the change actually is fixing the bug.

**test plan:**
`open fixtures/globals-with-create-react-fragment.html`
`open fixtures/globals-with-react-linked-input.html`
`open fixtures/requirejswith-create-react-fragment.html`
`open fixtures/requirejswith-react-linked-input.html`
`cd fixtures/webpack-aliaswith-create-react-fragment/ && yarn build && open index.html`
`cd fixtures/webpack-aliaswith-react-linked-input/ && yarn build && open index.html`

**issue:**
#9765

* Fix missing `React` in `react-linked-input` and `create-fragment`

**what is the change?:**
Manually tweaking the UMD builds for both add-ons to include a
dependency on React.

**why make this change?:**
They were broken before for AMD and CommonJS.
For reasons I have not debugged, the CommonJS builds are still broken,
but the AMD is now fixed and globals still work:

```
    do 'react-linked-input' and
    'create-react-fragment' work?

                before      after
              + my        + my        +
  en^ironment | fix       | fix       |
+----------------------------------------
              |           |           |
  Global JS   |  :) yes   |  :) yes   |
+----------------------------------------
              |           |           |
  AMD         |  X no     |  :) yes   |
+----------------------------------------
              |           |           |
  CommonJS    |  X no     |  X no     |
+-------------+-----------+-----------+--

```

**test plan:**
In the previous commit we set up fixtures to manually test these.

**issue:**
#9765

* More adjustments to enable testing with fixtures

Not worth explaining - just committing as a 'save point' while I fiddle
with the fixtures.

* Remove all cruft from manually testing addons in fixtures

**what is the change?:**
We forked three of the fixtures into two variations to test two of the
react addons. We also added `console.log` statements to the addons to
verify that we were loading the right build.

This commit cleans it up by
- deleting forked fixtures
- re-adding the original fixtures
- removing `console.log` statements

**why make this change?:**
To get this branch ready for review.

**test plan:**
`cd fixtures && node ./build-all.js` and then check the updated fixtures
manually

**issue:**
#9765

* Double to single quotes in 'react-linked-input'

**what is the change?:**
`:%s /"/'/gc`

I left double quotes wrapping cases where we have single quotes in the
string.

**why make this change?:**
I ran the code for the unminified 'react-linked-input' through
'prettier' so it would be easier for me to manually fix the UMD wrapper.
And 'prettier' changed many single quotes into double quotes. @spicyj
pointed out this will be treated differently by the google closure
compiler, and may have semantic differences.

**test plan:**
It's not worth testing imo.

**issue:**
#9765

* remove random newline
flarnie added a commit to flarnie/react that referenced this issue Jun 12, 2017
**what is the change?:**
We ran the latest version of
`addons/create-react-class/create-react-class.js` through https://jscompress.com/

**why make this change?:**
The last corner case I'm thinking of is this:
 - The `createClass` deprecation warning never went out in 15.5, and is going out now for real in 15.6.
 - The `createClass` UMD build is broken for AMD/CommonJS, but we fixed it. But not for the minified version of the file.
 - If someone see the warning, and tries to use the UMD build, then it's going to be broken in some cases.
 - Since we're skipping mentioning the add-ons in the blog post, and this might be a new warning for them, this could be a nasty surprise for folks.

We can do a quick 15.5.4 release of that package, we would at least fix that case.

This diverges from what @gaearon is doing to fix the add-ons. I would
probably try to use cherry-pick and interactive rebase to move this
commit to right after
facebook@ce3ecfb
and then do the patch release of `create-react-class` from that spot in
history.

Alternately I can merge this into the forked branch
`15.6-before-addon-reconstruction` and then do the patch release from
there.

**test plan:**
I didn't test this. Ideally minifying this file won't break anything,
and it's high cost to test add-ons until we have some repeatable tests
in place.

**issue:**
facebook#9765
flarnie added a commit that referenced this issue Jun 12, 2017
**what is the change?:**
We ran the latest version of
`addons/create-react-class/create-react-class.js` through https://jscompress.com/

**why make this change?:**
The last corner case I'm thinking of is this:
 - The `createClass` deprecation warning never went out in 15.5, and is going out now for real in 15.6.
 - The `createClass` UMD build is broken for AMD/CommonJS, but we fixed it. But not for the minified version of the file.
 - If someone see the warning, and tries to use the UMD build, then it's going to be broken in some cases.
 - Since we're skipping mentioning the add-ons in the blog post, and this might be a new warning for them, this could be a nasty surprise for folks.

We can do a quick 15.5.4 release of that package, we would at least fix that case.

This diverges from what @gaearon is doing to fix the add-ons. I would
probably try to use cherry-pick and interactive rebase to move this
commit to right after
ce3ecfb
and then do the patch release of `create-react-class` from that spot in
history.

Alternately I can merge this into the forked branch
`15.6-before-addon-reconstruction` and then do the patch release from
there.

**test plan:**
I didn't test this. Ideally minifying this file won't break anything,
and it's high cost to test add-ons until we have some repeatable tests
in place.

**issue:**
#9765
@gaearon gaearon closed this as completed Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants