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

Updates @babel/plugin-transform-modules-umd to 7.7.0+ #17818

Closed
wants to merge 1 commit into from

Conversation

MicahZoltu
Copy link

Summary

This fixes a bug in babel that was producing UMD modules that were incompatible with ES6 module loader in browsers.

Test Plan

Ran yarn test and yarn test-prod. There were 3 failures in yarn test-prod that make no sense to me (external developer) and I'm hoping someone can give feedback on how to address them in this PR description.

Fixes #17352

This fixes a bug in babel that was producing UMD modules that were incompatible with ES6 module loader in browsers.
@MicahZoltu
Copy link
Author

Summary of all failing tests
 FAIL  packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js (7.416s)
  ● ReactTestUtils.act() › concurrent mode › warn in prod mode › warns if you try to use act() in prod mode

    expect(spy).toHaveBeenCalledTimes(expected)

    Expected number of calls: 1
    Received number of calls: 0

      798 | 
      799 |         if (!__DEV__) {
    > 800 |           expect(console.error).toHaveBeenCalledTimes(1);
          |                                 ^
      801 |           expect(console.error.calls.argsFor(0)[0]).toContain(
      802 |             'act(...) is not supported in production builds of React',
      803 |           );

      at Object.<anonymous> (packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js:800:33)

  ● ReactTestUtils.act() › legacy mode › warn in prod mode › warns if you try to use act() in prod mode

    expect(spy).toHaveBeenCalledTimes(expected)

    Expected number of calls: 1
    Received number of calls: 0

      798 | 
      799 |         if (!__DEV__) {
    > 800 |           expect(console.error).toHaveBeenCalledTimes(1);
          |                                 ^
      801 |           expect(console.error.calls.argsFor(0)[0]).toContain(
      802 |             'act(...) is not supported in production builds of React',
      803 |           );

      at Object.<anonymous> (packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js:800:33)

  ● ReactTestUtils.act() › blocking mode › warn in prod mode › warns if you try to use act() in prod mode

    expect(spy).toHaveBeenCalledTimes(expected)

    Expected number of calls: 1
    Received number of calls: 0

      798 | 
      799 |         if (!__DEV__) {
    > 800 |           expect(console.error).toHaveBeenCalledTimes(1);
          |                                 ^
      801 |           expect(console.error.calls.argsFor(0)[0]).toContain(
      802 |             'act(...) is not supported in production builds of React',
      803 |           );

      at Object.<anonymous> (packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js:800:33)

If someone can walk me through what these errors mean I can try to fix them.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 86e766c:

Sandbox Source
broken-meadow-2ujfu Configuration

@sizebot
Copy link

sizebot commented Jan 10, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 86e766c

@sizebot
Copy link

sizebot commented Jan 10, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 86e766c

@MicahZoltu
Copy link
Author

Is there a way I can see the build artifacts from the CI build step? I cannot build locally because the build process appears to require Java, but I would like to take a look at the build output to make sure the target bug is actually fixed.

@bvaughn
Copy link
Contributor

bvaughn commented Jan 10, 2020

@MicahZoltu You can use the download script for this.

https://github.com/facebook/react/blob/master/scripts/release/README.md#prepare-canary

I think the command to pull down the artifacts for this PR should be:

cd scripts/release/
yarn install
./prepare-canary.js --build=75597

@MicahZoltu
Copy link
Author

react\scripts\release> node .\prepare-canary.js --build=75597
internal/modules/cjs/loader.js:797
    throw err;
    ^

Error: Cannot find module 'folder-hash'
Require stack:
- C:\Users\micah\Source\react\scripts\release\utils.js
- C:\Users\micah\Source\react\scripts\release\prepare-canary.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:794:15)
    at Function.Module._load (internal/modules/cjs/loader.js:687:27)
    at Module.require (internal/modules/cjs/loader.js:849:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at Object.<anonymous> (C:\Users\micah\Source\react\scripts\release\utils.js:5:23)
    at Module._compile (internal/modules/cjs/loader.js:956:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:973:10)
    at Module.load (internal/modules/cjs/loader.js:812:32)
    at Function.Module._load (internal/modules/cjs/loader.js:724:14)
    at Module.require (internal/modules/cjs/loader.js:849:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    'C:\\Users\\micah\\Source\\react\\scripts\\release\\utils.js',
    'C:\\Users\\micah\\Source\\react\\scripts\\release\\prepare-canary.js'
  ]
}

Same error if executed from project root using relative script path:

react> node .\scripts\release\prepare-canary.js

If it matters, I'm on running on Windows.

@MicahZoltu
Copy link
Author

yarn build fails after generating build/react.development.js and it appears that file doesn't have the UMD module fix in it. Also, looking at this project it appears it isn't using @babel/plugin-transform-modules-umd so it is unclear to me what is generating the UMD module during the build process. I assumed it was Babel since the function signature looks exactly like a babel signature, but it is possible that you all have something else generating the UMD header and you just copied the (old) babel UMD header?

Or perhaps there is another part of Babel that generates the UMD file (other than the UMD transformer) and it has the same bug as the UMD transformer had?

@MicahZoltu
Copy link
Author

Ahh, digging some more it appears you are actually using rollup in the build script, which presumably calls through to babel. I bet rollup has the same bug.

@MicahZoltu
Copy link
Author

Closing this PR since the problem appears to be rollup, not babel. I believe rollup fixed this bug long ago (over a year) but this project appears to be using an ancient version of rollup and upgrading is likely to be beyond my capability given my unfamiliarity with React's build process or rollup. I'll update the linked issue with additional details.

@MicahZoltu MicahZoltu closed this Jan 10, 2020
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.

Upgrade Rollup to 0.68.1+
4 participants