Fix missing react in create-react-class #9761

mondwan · 2017-05-24T10:30:27Z

gaearon · 2017-05-24T10:36:02Z

Note: we'll need to figure out how to recompile this file for .min.js to also have this change.

flarnie · 2017-05-24T13:36:24Z

I believe CI is failing because this was made against 15-stable, and CI is still failing there.

flarnie · 2017-05-24T14:23:59Z

This is close to being mergable - I think we are a bit worried about how this change could affect the other non-AMD uses. What if we model this after the way that react-dom works? (credit to @gaearon for thinking of this):

Then I think we could skip the var React = require('react'); in the body of the addon.

flarnie · 2017-06-07T23:25:29Z

@mondwan I'm going to pull this down and do some testing, and might add a commit to get it working for all builds.

mondwan · 2017-06-08T00:13:28Z

Great. Let me know if there are anything I can help. 2017年6月8日 07:27，"Flarnie Marchan" <notifications@github.com>寫道： @mondwan <https://github.com/mondwan> I'm going to pull this down and do some testing, and might add a commit to get it working for all builds. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9761 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADuAo8IX1xxUA8a7vTrhXc2U1MWINY1Bks5sBzHNgaJpZM4Nk6KW> .

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

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

This already was merged (facebook#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`

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

@mondwan

**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 · 2017-06-10T00:32:26Z

Almost have this working - but in general we are going to need a more maintainable way of updating add-ons going forward.

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

**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

flarnie · 2017-06-10T02:03:10Z

Pushed some commits on top of the original fix - here is an explanation:
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
               |            |            |
               +            +            +

I tested the fix manually using the fixtures - can explain if needed.

@gaearon would be interested in your ideas about this. Looks like we could take this approach to fix the other add-ons. I think we could land this, then update the minified 'create-react-class' and fix the other add-ons in a separate PR.

mondwan · 2017-06-10T05:12:09Z

Hello, it seems this line should also be removed?

https://github.com/mondwan/react/blob/15-stable/addons/create-react-class/create-react-class.js#L20

flarnie · 2017-06-10T17:24:46Z

addons/create-react-class/create-react-class.js

@@ -17,9 +17,10 @@
    if (typeof g.React === "undefined") {
      throw Error('React module should be required before createClass');
    }
-    g.createReactClass = f();
+    console.log('TEST');


will remove this

flarnie · 2017-06-10T17:32:36Z

CI is going to fail because it is always currently failing on the 15-stable branch. That will be fixed by the 15.6 release.

Here are my thoughts -

Merge this
cherry-pick it into 15.6-dev
fix the similar issue in create-fragment and react-linked-input (see Set 'React' as dependency in AMD builds of addons #9765) on 15.6-dev
merge those fixes (hopefully this weekend)
the fixes will be included in the 15.6 release on Tuesday, June 13th.

@mondwan

* 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

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

@spicyj

* 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

mondwan · 2017-06-19T03:45:23Z

Hey @flarnie , I have a question outside for this ticket's scope.

May I know how to pop out below contents in github?

flarnie reviewed 8 days ago View changes
Show outdated addons/create-react-class/create-react-class.js

It is useful for highlighting that particular line with a review icon. How to click such thing from the UI panel in github?

Fix missing react in create-react-class

1c303da

refs facebook#9689

facebook-github-bot added the CLA Signed label May 24, 2017

gaearon mentioned this pull request May 24, 2017

React 15.6 Umbrella #9398

Closed

49 tasks

flarnie self-requested a review May 24, 2017 14:24

flarnie added 2 commits June 9, 2017 17:20

flarnie reviewed Jun 10, 2017

View reviewed changes

remove stray console.log

789967f

flarnie approved these changes Jun 10, 2017

View reviewed changes

flarnie merged commit fc542d7 into facebook:15-stable Jun 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing react in create-react-class #9761

Fix missing react in create-react-class #9761

mondwan commented May 24, 2017

gaearon commented May 24, 2017

flarnie commented May 24, 2017

flarnie commented May 24, 2017

flarnie commented Jun 7, 2017

mondwan commented Jun 8, 2017 via email

flarnie commented Jun 10, 2017

flarnie commented Jun 10, 2017

mondwan commented Jun 10, 2017

flarnie Jun 10, 2017

flarnie commented Jun 10, 2017

mondwan commented Jun 19, 2017

Fix missing react in create-react-class #9761

Fix missing react in create-react-class #9761

Conversation

mondwan commented May 24, 2017

gaearon commented May 24, 2017

flarnie commented May 24, 2017

flarnie commented May 24, 2017

flarnie commented Jun 7, 2017

mondwan commented Jun 8, 2017 via email

flarnie commented Jun 10, 2017

flarnie commented Jun 10, 2017

mondwan commented Jun 10, 2017

flarnie Jun 10, 2017

Choose a reason for hiding this comment

flarnie commented Jun 10, 2017

mondwan commented Jun 19, 2017