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

Production dependencies update #4643

Merged
merged 2 commits into from Jun 7, 2021
Merged

Production dependencies update #4643

merged 2 commits into from Jun 7, 2021

Conversation

juergba
Copy link
Member

@juergba juergba commented Jun 1, 2021

Description

We update the production dependencies for our next major release v.9.0.0.

I could not upgrade following packages:

  • escape-string-regexp
  • log-symbols
  • supports-color

In there latest major releases these are pure ESM packages now. Since Mocha is a CJS package we can't simply replace the synchronous require() by the asynchronous import(). A challenge to be resolved ...


After rebuilding package-lock.json the browser tests failed with:

Uncaught TypeError: Cannot read property 'mark' of undefined
  at mocha.js:24469:71

  TypeError: Cannot read property 'mark' of undefined
      at mocha.js:24469:71
      at mocha.js:24496:3
      at mocha.js:4:92
      at mocha.js:5:2

and I had to remove the change of PR #4577 in rollup.config.js.
It's about regenerator-runtime which Babel uses to transpile async/await down to ES5.
We evtl. have to find another solution for the EvalError when using Mocha in browser environment.

Edit: At the end I did not revert PR #4577, but pinned the versions of our transpiling dependencies.

@juergba
Copy link
Member Author

juergba commented Jun 1, 2021

@giltayar How do we handle this when a dependency migrates from CJS to a pure ESM package?

@juergba juergba requested a review from a team June 1, 2021 13:55
@juergba juergba added the dependencies Pull requests that update a dependency file label Jun 1, 2021
@juergba juergba added this to the v9.0.0 milestone Jun 1, 2021
@juergba juergba self-assigned this Jun 1, 2021
@giltayar
Copy link
Contributor

giltayar commented Jun 1, 2021

@juergba there is no way to do this, other than to switch to an async path and use async import. It has been talked about extensively, and the Node.js Modules team is mostly saying in that this capability is NOT being planned, and will not happen.

I believe we should start thinking about migrating Mocha to be an async-only API in v10, which will allow us to use async import and thus all new ESM dependencies. Till then, we stick with the old dependencies that are CJS.

This movement towards ESM-only is starting to gain some momentum, with at least two important OSS developers saying they're going full ESM by the end of 2021 (Sindre Sorhus and Titus). I believe Mocha should start thinking about also migrating to ESM only, maybe by the time 12 EOLs.

Here are some of Sindre Sorhus' thoughts on this:

https://blog.sindresorhus.com/get-ready-for-esm-aa53530b3f77
https://blog.sindresorhus.com/hello-modules-d1010b4e777b

@juergba juergba force-pushed the juergba/dependencies branch 2 times, most recently from 1bcec3f to f0a9579 Compare June 5, 2021 17:47
@juergba
Copy link
Member Author

juergba commented Jun 5, 2021

@snoack I had to remove your EvalError solution of #4577.

When rebuilding package-lock.json (even without upgrading any dependencies) our browser tests failed due to regenerator-runtime, see above in the description. Maybe you find some time to have a look at this branch. I would also appreciate some help, in case we need another solution for this EvalError.

@snoack
Copy link
Contributor

snoack commented Jun 5, 2021

@juergba Apparently, they changed the boilerplate of regenerator-runtime again and now rely again on function () { return this; }() || Function("return this")());. So there is no simple workaround anymore, and we are back were we started with the following options:

  1. Don't update to a new version of regenerator-runtime in package-lock.json.
  2. Don't use async/await (and/or generator functions).
  3. Don't support IE (and remove transpilation through Babel).
  4. Have two separate builds, one with tanspiled code that works on IE, one that isn't transpiled and works without unsafe-eval in modern browsers.

@juergba
Copy link
Member Author

juergba commented Jun 6, 2021

@snoack thank you for your reply, 😀. I was hoping to get an easy answer.

Will this PR regenerator/396 offer any help as soon as merged?

For the moment I'm going for option 1).
With npm list -all regenerator-runtime I get, inspite having set "@babel/preset-env": "^7.12.17" in package.json:

mocha@8.4.0 D:\mocha-fork\mocha
+-- @babel/preset-env@7.14.4
| `-- @babel/plugin-transform-regenerator@7.13.15
|   `-- regenerator-transform@0.14.5
|     `-- @babel/runtime@7.14.0
|       `-- regenerator-runtime@0.13.7
`-- rewiremock@3.14.3
  `-- babel-runtime@6.26.0
    `-- regenerator-runtime@0.11.1

I will try to fix this first, and then set your hack back on duty.

@snoack
Copy link
Contributor

snoack commented Jun 6, 2021

Yes, regenerator/396 should fix the cause of this issue.

@juergba
Copy link
Member Author

juergba commented Jun 6, 2021

@snoack Pinning only the version of regenerator-runtime didn't do the trick, so I added five devdependencies to our package.json.
The result looks like this:

$ npm list regenerator-runtime

mocha@8.4.0 D:\mocha-fork\mocha
+-- @babel/runtime@7.12.5
| `-- regenerator-runtime@0.13.7  deduped
+-- regenerator-runtime@0.13.7
`-- rewiremock@3.14.3
  `-- babel-runtime@6.26.0
    `-- regenerator-runtime@0.11.1

Our tests have passed successfully now. Could you have a look, please?
This whole topic with babel/regenerator seems so fragile, and it's not maintained on a regular basis. I don't know ...

@giltayar
Copy link
Contributor

giltayar commented Jun 6, 2021

@juergba I feel you! I would seriously think of removing support for IE11 in v9. Whoever needs it can use Mocha v8, and it's getting EOL-d real soon now, so maintaining v8 for critical security vulnerabilities shouldn't be a problem.

(but this is just my 2 cents. I definitely don't understand all the implications)

@juergba
Copy link
Member Author

juergba commented Jun 6, 2021

I would seriously think of removing support for IE11 in v9.

IMO it's too early. But I'm starting to like the option with two seperate builds. For a short test I removed the transpiling step and locally the browser test succeeds. The bundled file is much smaller (20k lines instead of 30k).
So for now we could publish v9.0.0 as is with:

  • mocha.js: bundled and transpiled to ES5

and for later v9.* releases we add a second bundle:

  • mocha-neo.js: bundled with 2018 (or whatever name)

We publish both files in our package on npm. www.unpkg.com is exposing our entire package, so both bundles would be available without breaking change.

Edit: our browser tests are week, only units and no integration tests. So I don't really know wether modern browsers support ES2018 completely.

@snoack
Copy link
Contributor

snoack commented Jun 6, 2021

Pinning only the version of regenerator-runtime didn't do the trick, so I added five devdependencies to our package.json.

Only pinning @babel/preset-env seems to be sufficient without explicitly adding the other 4 packages to package.json.

Our tests have passed successfully now. Could you have a look, please?

Looks good. FWIW as I said before you can easily test this scenario by adding <meta http-equiv="Content-Security-Policy" content="script-src http: https: 'unsafe-inline'"> to the test page. Maybe it would make sense to do so in the automated tests?

But I'm starting to like the option with two seperate builds.

Note that this would technically be a breaking change, since users would have to change the bundle name when udating to a newer version of Mocha (and then once again if you ever plan to rename mocha-neo.js, e.g. in order to drop the suffix).

@juergba
Copy link
Member Author

juergba commented Jun 7, 2021

Only pinning @babel/preset-env seems to be sufficient [...]

I tried and it didn't work, since @babel/preset-env doesn't pin its sub-dependencies. I don't know wether we need all five packages though.

FWIW as I said before you can easily test this scenario [...]

How can I run this test with my local bundle of mocha.js?
I get the error: Refused to load the script 'file:///C:/Users/.../mocha.js'

@juergba juergba merged commit 7259a5b into master Jun 7, 2021
@juergba juergba deleted the juergba/dependencies branch June 7, 2021 13:51
@juergba juergba added area: browser browser-specific semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Jun 7, 2021
@snoack
Copy link
Contributor

snoack commented Jun 7, 2021

I tried and it didn't work, since @babel/preset-env doesn't pin its sub-dependencies. I don't know wether we need all five packages though.

Hmm, I tried it, and it did work. Note that @babel/preset-env kinda pins it dependencies, i.e. it requires "@babel/plugin-transform-regenerator": "^7.12.13" which means >= 7.12.13, < 8.

How can I run this test with my local bundle of mocha.js?

Did you try adding file: to the Content-Security-Policy's allowed sources?

<meta http-equiv="Content-Security-Policy" content="script-src http: https: file: 'unsafe-inline'">

@juergba
Copy link
Member Author

juergba commented Jun 8, 2021

@snoack
re pinning @babel/preset-env: I'm using npm v6.14.13, maybe the version matters. When rebuilding package-lock.json I always delete the node-modules folder and the old package-lock.json, then npm install. But maybe I'm wrong, I will try again another day.

Did you try adding file: to the Content-Security-Policy's allowed sources?

Works, excellent!

Thanks for your help. 👍

@juergba
Copy link
Member Author

juergba commented Jul 29, 2021

@snoack a few days ago a new release v0.13.9 of regenerator-runtime has been published with this commit.

I applied following changes:

  • upgraded @babel/preset-env to v7.14.8
  • removed the version-pinned devDependencies
  • removed your var regeneratorRuntime hack

npm list -all regenerator-runtime shows:

+-- @babel/preset-env@7.14.8
| `-- @babel/plugin-transform-regenerator@7.14.5
|   `-- regenerator-transform@0.14.5
|     `-- @babel/runtime@7.14.8
|       `-- regenerator-runtime@0.13.9

But the test you gave me, fails with Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source [...].

Do you have any tip? Or is v0.13.9 just not the solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: browser browser-specific dependencies Pull requests that update a dependency file semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants