Fix missing react dependency in some addon umd builds #9919

flarnie · 2017-06-11T00:53:33Z

We absolutely need a build process and tests before making any further changes to these addons.

That said, this is a quick fix to hopefully improve the situation in conjunction with the 15.6 release. We will probably continue working on fixing the add-on packages after 15.6.

The fix:

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

The process for testing the 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

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

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

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

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

Not worth explaining - just committing as a 'save point' while I fiddle with the 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:** facebook#9765

sophiebits

Unfortunately we don't have a test suite that can run entirely against our public API. If we did, we could run all our tests against each type of build. It is probably a surmountable hurdle to make it happen though.

sophiebits · 2017-06-11T03:59:33Z

addons/react-linked-input/react-linked-input.js

+
+    g.LinkedInput = f(g.React);
+  }
+})(function(React) {


nit: why is this fully expanded while the previous one is in one line? a little easier to compare them if they match style

sophiebits · 2017-06-11T03:59:45Z

fixtures/webpack/input.js

@@ -1,3 +1,4 @@
+


sophiebits · 2017-06-11T04:09:44Z

addons/react-linked-input/react-linked-input.js

+          var warning = require("fbjs/lib/warning");
+
+          var hasReadOnlyValue = {
+            button: true,


Maybe you ran prettier over all of this? The quotes here changed. It is not a big deal but would be better to keep the original version in case there are slight semantic differences (closure compiler advanced probably doesn't work properly on this code anyway, but it does treat these differently).

I did run it through prettier - was wary of trying to take specific parts of the changes because I might miss a closing bracket somewhere.

I can fix the quotes at least though.

@spicyj

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

sophiebits · 2017-06-12T16:13:56Z

It's actually quotes vs no quotes in object keys and property accesses which makes a difference, not double vs single. (Is it difficult to un-prettier it?)

flarnie · 2017-06-12T16:25:22Z

I can fix the object keys, and how would I un-prettier it? I ran it through prettier and then made changes, so reverting the whole thing would lose the changes I made by hand. And I couldn't see where to make the changes without prettifying it.

flarnie added 7 commits June 10, 2017 11:36

More adjustments to enable testing with fixtures

862b57d

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

flarnie added this to the 15.6 milestone Jun 11, 2017

flarnie requested review from bvaughn, gaearon and acdlite June 11, 2017 00:53

facebook-github-bot added the CLA Signed label Jun 11, 2017

flarnie force-pushed the fixMissingReactDependencyInSomeAddonUMDBuilds branch from 3eebf12 to 5f30b23 Compare June 11, 2017 00:54

sophiebits approved these changes Jun 11, 2017

View reviewed changes

flarnie added 2 commits June 12, 2017 07:38

remove random newline

3d6c422

flarnie merged commit 3c89893 into facebook:15.6-dev Jun 12, 2017

flarnie deleted the fixMissingReactDependencyInSomeAddonUMDBuilds branch May 25, 2018 17:52

This was referenced Aug 26, 2019

[Snyk] Upgrade react from 15.4.2 to 15.6.2 T0mmykn1fe/blueocean-plugin#3

Open

[Snyk] Upgrade react-dom from 15.4.2 to 15.6.2 T0mmykn1fe/blueocean-plugin#4

Open

This was referenced Jan 23, 2020

[Snyk] Upgrade react from 15.3.2 to 15.6.2 anyulled/todo-react-redux#1

Merged

[Snyk] Upgrade react-dom from 15.3.2 to 15.6.2 anyulled/todo-react-redux#2

Merged

snyk-bot mentioned this pull request Feb 9, 2020

[Snyk] Upgrade react from 15.4.1 to 15.6.2 NetanelHadas/react-native-albums#1

Open

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade react-dom from 15.4.0 to 15.6.2 saurabharch/svgs#1

Open

This was referenced Feb 23, 2020

[Snyk] Upgrade react from 15.4.0 to 15.6.2 saurabharch/svgs#2

Open

[Snyk] Upgrade react-dom from 15.4.0 to 15.6.2 raindigi/svgs#24

Open

[Snyk] Upgrade react from 15.4.0 to 15.6.2 raindigi/svgs#25

Open

snyk-bot mentioned this pull request Mar 3, 2020

[Snyk] Upgrade: react, react-dom judynjagi/Bucketlist-React-Redux-WebApplication#1

Open

This was referenced Mar 18, 2020

[Snyk] Upgrade: react, react-dom O330oei/sharetribe#3

Merged

[Snyk] Upgrade: react, react-dom godaddy/svgs#51

Closed

[Snyk] Upgrade react from 15.3.1 to 15.6.2 ImranSaadi/react-native-admob#10

Merged

This was referenced Mar 27, 2020

[Snyk] Upgrade: react, react-dom karlhorky/talks#12

Closed

[Snyk] Upgrade: react, react-dom dmitriz/react-pure-component-starter#1

Open

This was referenced Apr 7, 2020

[Snyk] Upgrade react from 15.2.1 to 15.6.2 rangle/react-native-workshop-1#1

Open

[Snyk] Upgrade react from 15.0.2 to 15.6.2 rangle/react-native-workshop#64

Open

[Snyk] Upgrade react from 15.3.1 to 15.6.2 rangle/NavigationExperimentalWorkshop#2

Open

This was referenced May 6, 2020

[Snyk] Upgrade: react, react-dom yamafaktory/babel-react-rollup-starter#117

Open

[Snyk] Upgrade: react, react-dom yamafaktory/buble-react-rollup-starter#95

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing react dependency in some addon umd builds #9919

Fix missing react dependency in some addon umd builds #9919

flarnie commented Jun 11, 2017 •

edited

Loading

sophiebits left a comment

sophiebits Jun 11, 2017

sophiebits Jun 11, 2017

sophiebits Jun 11, 2017

flarnie Jun 12, 2017

flarnie Jun 12, 2017

sophiebits commented Jun 12, 2017

flarnie commented Jun 12, 2017

Fix missing react dependency in some addon umd builds #9919

Fix missing react dependency in some addon umd builds #9919

Conversation

flarnie commented Jun 11, 2017 • edited Loading

The fix:

The process for testing the fix:

sophiebits left a comment

Choose a reason for hiding this comment

sophiebits Jun 11, 2017

Choose a reason for hiding this comment

sophiebits Jun 11, 2017

Choose a reason for hiding this comment

sophiebits Jun 11, 2017

Choose a reason for hiding this comment

flarnie Jun 12, 2017

Choose a reason for hiding this comment

flarnie Jun 12, 2017

Choose a reason for hiding this comment

sophiebits commented Jun 12, 2017

flarnie commented Jun 12, 2017

flarnie commented Jun 11, 2017 •

edited

Loading