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

fix: add browser condition to package.json #616

Closed
wants to merge 4 commits into from
Closed

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Feb 10, 2022

jest@28 will ship with exports support. But when using jest-environment-jsdom to simulate a browser environment, we don't provide the node condition, meaning it resolves to default. Unless ESM is enabled, this results in a syntax error.

@ctavan ctavan mentioned this pull request Feb 11, 2022
Copy link
Member

@ctavan ctavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for raising this!

I think the patch looks correct and non-breaking but I'm still trying to fully understand the consequences, in particular if this will change anything about how webpack and rollup will behave.

A good first sign that the webpack and rollup behaviors remain unchanged is that the browser tests still pass with your patch.

I believe those tests cover the path where webpack and rollup are instructed to use the esm-browser build.

Where I'm not entirely sure is cases, where webpack/rollup would pick the pkg.exports.browser.require path. I think those cases, the overrides in pkg.browser must be taken into account by the bundlers in order to generate a browser-compatible build.

Have you tested this case?

package.json Outdated Show resolved Hide resolved
@SimenB
Copy link
Contributor Author

SimenB commented Feb 11, 2022

Where I'm not entirely sure is cases, where webpack/rollup would pick the pkg.exports.browser.require path. I think those cases, the overrides in pkg.browser must be taken into account by the bundlers in order to generate a browser-compatible build.

Have you tested this case?

Nope, not tested. However, I don't think those fields are related? At least with main, it is 100% ignored if exports is present, I'd assume browser was the same. So older tools without exports support use browser while newer ignores it. Might be wrong, tho 😀

EDIT: Yep, at least webpack behaves in this way: https://webpack.js.org/guides/package-exports/#notes-about-ordering

exports field is preferred over other package entry fields like main, module, browser or custom ones.


Might have misunderstood your question, are you worried about webpack getting the require entry? Not sure why it'd be an issue if webpack picked it? I assume it'll pass the import condition today, in which case it's the same as before (as it was default). If they only pass require that seems correct to not return ESM.

@ctavan
Copy link
Member

ctavan commented Feb 11, 2022

I just ran a quick test: 13ceb79

When using require() webpack picks ./dist/index.js as an entry point and then applies the overrides from pkg.browser, i.e. it uses ./dist/rng-browser.js etc.

I have not verified with rollup yet. But at least for webpack, the pkg.browser field is not ignored, however after all it does the right thing.

I think this should be covered in the browser tests. I can't promise when I can find the time to do this though.

Edit: The test in 13ceb79 is not complete, I added console.log() statements manually in the local ./dist output files to verify that webpack indeed picked ./dist/rng-browser.js etc…

@SimenB
Copy link
Contributor Author

SimenB commented Feb 11, 2022

I have not verified with rollup yet. But at least for webpack, the pkg.browser field is not ignored, however after all it does the right thing.

Their docs are wrong, then? Seems weird 😅 But if it works like you want it to, all is well?

@SimenB
Copy link
Contributor Author

SimenB commented Feb 11, 2022

Ah, seems like browser is just "aliases", it has nothing directly to do with export conditions. So webpack applies additional mapping after resolving the import: https://webpack.js.org/configuration/resolve/#resolvealiasfields

@broofa
Copy link
Member

broofa commented Feb 11, 2022

Ah, seems like browser is just "aliases", it has nothing directly to do with export conditions

Is it even valid to have a "browser" field nested w/in the "exports" block like this? If so, where might we find some sort of description or spec for how it's interpreted?

@SimenB
Copy link
Contributor Author

SimenB commented Feb 11, 2022

https://nodejs.org/api/packages.html#nested-conditions

browser I was talking about in that comment is a browserify thing (at the top level), not the export condition I'm adding in this PR

@ctavan
Copy link
Member

ctavan commented Feb 11, 2022

Ah, seems like browser is just "aliases", it has nothing directly to do with export conditions

Is it even valid to have a "browser" field nested w/in the "exports" block like this? If so, where might we find some sort of description or spec for how it's interpreted?

As @SimenB mentioned the pkg.browser field is different from the browser environment in pkg.exports. Indeed, pkg.browser was introduced by browserify a loooong time ago and we've been using it for ages here as well. However, pkg.exports also allows a browser environment which, according to https://webpack.js.org/guides/package-exports/#target-environment seems to be rather widely supported.

However what I don't really like about the current approach in this PR is that we're mixing two concepts: Through pkg.exports.browser.require we're telling bundlers to use the Node.js CommonJS build at ./dist/index.js and then we're relying on the independent, legacy concept of pkg.browser for the browser specific overrides over the Node.js CommonJS build.

I think a cleaner solution would be to provide a consistent CommonJS build for the browser, e.g. in ./dist/commonjs-browser/index.js. Folks have run into similar issues in the past (1, 2, 3, 4) when misconfiguring their bundlers… I would still argue that this is always an unintended misconfiguration and should not be done, folks should not choose the CommonJS builds of their dependencies for browser builds, they should use the ESM build that natively supports tree shaking instead.

However in this case now with jest-environment-jsdom where a browser env is only simulated within a Node.js process things may actually start looking differently. Thinking about this, however, uncovers an interesting aspect: I believe with this current patch and when using jest-environment-jsdom jest may actually end up executing the CommonJS Node.js code and not the browser code. In practice, this will probably produce the intended results for the test cases. However it's still possible that such test cases will pass with Jest but would fail in a browser since browsers will effectively execute different code.

I'm sorry for the long write up, I guess the main message is that this is complicated

@SimenB do you have an example of where a syntax error is thrown (what you reference in the original PR description)? I would really like to figure out what happens under the hood when jest runs these tests.

@SimenB
Copy link
Contributor Author

SimenB commented Feb 11, 2022

A browser build in CJS would be even better of course 🙂

@SimenB do you have an example of where a syntax error is thrown (what you reference in the original PR description)? I would really like to figure out what happens under the hood when jest runs these tests.

Jest provides ['require', 'browser', 'default'] as conditions, meaning default is returned, which is ./dist/esm-browser/index.js.

https://github.com/SimenB/jest-uuid

Remove comment at top of test.js to see it pass (as Jest then runs in node env and provides node condition instead of browser)

@SimenB
Copy link
Contributor Author

SimenB commented Feb 17, 2022

@ctavan how do we unblock this? 🙂 This issue is actually stopping me from using the Jest alpha at work 😅 (although I guess I can just patch package.json, but still)

@ctavan
Copy link
Member

ctavan commented Feb 17, 2022

Sorry for the silence and sorry to hear this blocks you.

I'm happy to help unblock this but I have to admit that I'm not sure what the right way forward is. I experimented with building a browser commonjs build, but that only reveals the actual problem…

If you run jest on jsdom.test.js in https://github.com/uuidjs/uuid/tree/simenb-patch/examples/jest-uuid-main you get:

$ ./node_modules/.bin/jest jsdom.test.js
  console.log
    commonjs-browser RNG

      at Object.<anonymous> (../../dist/commonjs-browser/rng.js:3:9)

 FAIL  ./jsdom.test.js
  ● Test suite failed to run

    crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported

      20 |
      21 |     if (!getRandomValues) {
    > 22 |       throw new Error('crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported');
         |             ^
      23 |     }
      24 |   }
      25 |

      at rng (../../dist/commonjs-browser/rng.js:22:13)
      at Object.v4 (../../dist/commonjs-browser/v4.js:23:32)
      at Object.<anonymous> (jsdom.test.js:5:18)

So the browser commonjs build uses browser APIs (crypto), it has to do this, otherwise we can't sell that as a browser build. However those APIs don't seem to be provided/mocked by jsdom.

I'm currently failing to understand what the expectations are when you run a test case with @jest-environment jsdom? How close to a browser environment is this supposed to be?

If it's not reasonably close, I think that resolving the pkg.exports.browser target for @jest-environment jsdom tests will inevitably cause many problems like the one above, where the code that is offered on the pkg.exports.browser export relies on native browser APIs.

Can you help clarify the expectations?

Edit: So to reiterate: Your patch only "works" because it effectively makes jest execute the node build instead of the browser build.

@SimenB
Copy link
Contributor Author

SimenB commented Feb 18, 2022

So the browser commonjs build uses browser APIs (crypto), it has to do this, otherwise we can't sell that as a browser build. However those APIs don't seem to be provided/mocked by jsdom.

That seems perfectly fine to me? "If you want to use uuid in an "old" browser, you need to polyfill". I don't see the issue with that.

I'm currently failing to understand what the expectations are when you run a test case with @jest-environment jsdom? How close to a browser environment is this supposed to be?

As close as possible. If APIs are missing from the env, the module should complain. The fact I want to use CJS instead of ESM is separate from browser vs node. It's not up to this module to say "you cannot use require in the browser", it should just say "here's a browser entry point for CJS" and it's up to the consumer to ensure their environment works for what they're trying to load.


Or uuid can of course continue to say "there is no CJS version for the browser", that's also perfectly valid (and is the situation today, it works as you say by happy coincidence since Jest loads main). 🙂 That just means people using Jest and uuid in a browser needs to find alternatives or mock the dependency, just like with any other dependency. Nothing new or weird about that.

@ro-savage
Copy link

Jest 28 has now been released - which means that Jest and UUID are no longer compatible.

For anyone trying to fix this issue - @dbjorge did a good temp solution here: microsoft/accessibility-insights-web#5421 (comment)

But would be great to have a proper solution to this.

@ctavan
Copy link
Member

ctavan commented Apr 26, 2022

Looping in @dbjorge here since I think this thread is more suitable to discuss the jest/uuid specific issue than #616 (comment).

First of all, thanks for the great summary @dbjorge!

At the risk of repeating what @dbjorge already summarized perfectly well, I'd like to highlight again:

Jest + UUID just "accidentally" worked in the past because Jest used to pick the Node.js build even in environments like jsdom that were supposed to mimic the browser. Apparently this has worked well for folks in practice, so I think a custom jest resolver like this one is actually a great mitigation for the time being as it just puts us back to where we were before Jest 28.

If we were adding a CommonJS browser build for completeness of the support matrix of uuid, we would still need something like jsdom/jsdom#3352 to make this work with Jest 28. And I stand with my statement that I generally think CommonJS for the browser has no production use case. Requiring a whole new build specifically for testing purposes which has otherwise no production use case to me sounds more like something that should be fixed in the testing library rather than in the libraries being tested… So to that end I'd prefer see ESM support land in Jest.

@SimenB two questions:

  1. Can you foresee if/when Jest will get ESM support?
  2. Assuming, 1. will materialize eventually, could Jest potentially ship fallback resolutions logic for widely spread libraries like uuid that would fall back to resolving the node build (essentially bundling the custom resolver that @dbjorge wrote?)?

I can see how this affects many people, given both jest and uuid are so widespread, I'm just still failing to clearly identify the right solution for this situation.

@dbjorge
Copy link

dbjorge commented Apr 27, 2022

@ctavan Thanks for the ping!

jestjs/jest#9430 is the meta-issue tracking ESM support in Jest. It is a very long and very soul-crushing read. For the purposes of our discussion here, I think it's probably safe to say that it won't happen soon enough for the folks that need Jest+uuid workarounds to unblock Jest 28 updates.

I want to emphasize that, as a user of both libraries, I don't think anyone on the uuid or Jest sides are making unreasonable decisions here; I don't really feel convinced that a workaround really "belongs" in either library. Particularly:

  • I think it's reasonable for the uuid maintainers to push back on adding a CommonJS + browser export just for the sake of one testing framework
  • I think the new Jest behavior to try to use browser exports from jest-environment-jsdom is an excellent change overall, and worth breaking this one case for
  • If I were a uuid maintainer, I would be very hesitant to touch its exports unnecessarily for fear of breaking backcompat with someone's weird webpack config
  • If I were a Jest maintainer, I would be very hesitant to establish a precedent of adding package-specific magic to Jest's default resolver

I think there are probably a large number of users that will be impacted by this, but more importantly than that, I want to recognize that neither Jest nor uuid have any obligation to be cross-compatible with one another. I am a thankful user of the work you have both made available for free, and neither project's maintainers owe me (and certainly not my employer) a damn thing; if I have to maintain a few lines of resolver.js to use them together, that's perfectly fine.

@SimenB
Copy link
Contributor Author

SimenB commented Apr 27, 2022

Hey! 👋

@SimenB two questions:

  1. Can you foresee if/when Jest will get ESM support?

There is support today, however it's experimental (docs). And given the complete lack of progress on the V8/Node side of things (nodejs/node#37648, AFAIK latest is the (seemingly) stalled https://chromium-review.googlesource.com/c/v8/v8/+/3172764) it will stay experimental (as in you need to run node with --experimental-vm-modules) for the foreseeable future, especially for e.g. node 14 which will likely never be unflagged at this point. So we're realistically talking at least a year (but since I would have said that a year ago... well 😅).

  1. Assuming, 1. will materialize eventually, could Jest potentially ship fallback resolutions logic for widely spread libraries like uuid that would fall back to resolving the node build (essentially bundling the custom resolver that @dbjorge wrote?)?

I don't think we'll be adding workarounds for any libraries directly in Jest. The easiest solution would probably be for somebody to create a jest-uuid that people can stick in the moduleNameMapper config. It could just do module.exports = require('uuid') and possibly be a cleaner solution than custom resolvers (as it composes better if people already use a custom resolver). Or potentially having some well maintained custom resolver people can plug in that looks like the one @dbjorge has built, but supporting more modules than just uuid. But the out of the box experience is unlikely to attempt to work around this issue.


I don't think anyone on the uuid or Jest sides are making unreasonable decisions here; I don't really feel convinced that a workaround really "belongs" in either library.

I 100% agree with this statement, well put!

@ctavan
Copy link
Member

ctavan commented Apr 27, 2022

Thanks for chiming in @SimenB!

I don't think we'll be adding workarounds for any libraries directly in Jest. The easiest solution would probably be for somebody to create a jest-uuid that people can stick in the moduleNameMapper config. It could just do module.exports = require('uuid') and possibly be a cleaner solution than custom resolvers (as it composes better if people already use a custom resolver).

@dbjorge what do you think about this approach? Would this be something you'd consider to test-drive on accessibility-insights-web?

Or potentially having some well maintained custom resolver people can plug in that looks like the one @dbjorge has built, but supporting more modules than just uuid.

Or maybe rather this second approach?

Given I currently don't have my own use case for this, I don't think I'll be able to provide a solution (like the jest-uuid wrapper), but I'd be happy to contribute (e.g. I'd be open to host such a module in the uuidjs github & npm org).

@dairyisscary
Copy link

For anyone coming here looking for a quick workaround, i did the following in my jest config (which i think is more or less @SimenB's suggestion with a jest-uuid)

moduleNameMapper: {
  "^uuid$": "<rootDir>/node_modules/uuid/dist/index.js",
},

@SimenB
Copy link
Contributor Author

SimenB commented Apr 27, 2022

Yep! If your config is in JS, I'd recommend "^uuid$": require.resolve("uuid") instead of hardcoding a path into node_modules, but that should work fine 🙂

@dbjorge
Copy link

dbjorge commented Apr 27, 2022

The easiest solution would probably be for somebody to create a jest-uuid that people can stick in the moduleNameMapper config. It could just do module.exports = require('uuid') and possibly be a cleaner solution than custom resolvers (as it composes better if people already use a custom resolver).

@dbjorge what do you think about this approach? Would this be something you'd consider to test-drive on accessibility-insights-web?

I actually tried basically this solution before I went with the custom resolver - I don't think a wrapper package is necessary, you can just do something like:

moduleNameMapper: {
  // This forces Jest/jest-environment-jsdom to use a Node+CommonJS version of uuid, not a Browser+ESM one
  // See https://github.com/uuidjs/uuid/pull/616
  //
  // WARNING: if your dependency tree has multiple paths leading to uuid, this will force all of them to resolve to
  // whichever one happens to be hoisted to your root node_modules folder. This makes it much more dangerous
  // to consume future uuid upgrades. Consider using a custom resolver instead of moduleNameMapper.
  '^uuid$': require.resolve('uuid'),
}

However, I rejected this in favor of the custom resolver because my understanding of the moduleNameMapper option is that it would essentially force every transitive dependency on uuid in my project to resolve to whichever version of uuid happens to be hoisted to the root node_modules folder. I'm already working in a project which has multiple versions of uuid in its dependency tree, but even if I wasn't, this scenario creates a maintenance time bomb if uuid ever releases a v9 with breaking changes. I preferred the custom resolver because it lets each dependency chain continue to use (the CommonJS version of) whatever uuid it was already using.

edit: beaten twice, oops

@SimenB
Copy link
Contributor Author

SimenB commented Apr 27, 2022

@dbjorge your analysis is correct though, this forces a single version across the entire tree, so might not be correct for all use cases. a resolver based solution is safer in that regard 🙂

@SimenB
Copy link
Contributor Author

SimenB commented Jul 18, 2022

Sorry for the silence and sorry to hear this blocks you.

I'm happy to help unblock this but I have to admit that I'm not sure what the right way forward is. I experimented with building a browser commonjs build, but that only reveals the actual problem…

If you run jest on jsdom.test.js in https://github.com/uuidjs/uuid/tree/simenb-patch/examples/jest-uuid-main you get:

$ ./node_modules/.bin/jest jsdom.test.js
  console.log
    commonjs-browser RNG

      at Object.<anonymous> (../../dist/commonjs-browser/rng.js:3:9)

 FAIL  ./jsdom.test.js
  ● Test suite failed to run

    crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported

      20 |
      21 |     if (!getRandomValues) {
    > 22 |       throw new Error('crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported');
         |             ^
      23 |     }
      24 |   }
      25 |

      at rng (../../dist/commonjs-browser/rng.js:22:13)
      at Object.v4 (../../dist/commonjs-browser/v4.js:23:32)
      at Object.<anonymous> (jsdom.test.js:5:18)

So the browser commonjs build uses browser APIs (crypto), it has to do this, otherwise we can't sell that as a browser build. However those APIs don't seem to be provided/mocked by jsdom.

I'm currently failing to understand what the expectations are when you run a test case with @jest-environment jsdom? How close to a browser environment is this supposed to be?

JSDOM v20 supports crypto.getRandomValues(), fwiw

@broofa
Copy link
Member

broofa commented Jul 18, 2022

JSDOM v20 supports crypto.getRandomValues(), fwiw

@SimenB I'm being lazy here (headed out the door to walk dog, get my day started), but does that mean things should "just work" now? ... or that work is still needed w/in this module to resolve this issue?

@SimenB
Copy link
Contributor Author

SimenB commented Jul 18, 2022

It means the browser version can be ported to CJS (so just module format change, no logic change needed (iirc))

@ctavan
Copy link
Member

ctavan commented Jul 29, 2022

I can confirm that this now works with jest-environment-jsdom@29.0.0-alpha.0 (which has jsdom@20.0.0 as a dependency).

ctavan added a commit that referenced this pull request Jul 29, 2022
Jest runs browser tests in a jsdom environment which now also supports
web crypto polyfills.

Since ESM support in Jest is currently still limited, it requires a
commonJS build for browser environments, see the discussion in
#616 for all the details.

Co-authored-by: Christoph Tavan <dev@tavan.de>
ctavan added a commit that referenced this pull request Jul 29, 2022
Jest runs browser tests in a jsdom environment which now also supports
web crypto polyfills.

Since ESM support in Jest is currently still limited, it requires a
commonJS build for browser environments, see the discussion in
#616 for all the details.

Co-authored-by: Christoph Tavan <dev@tavan.de>
ctavan added a commit that referenced this pull request Jul 29, 2022
Jest runs browser tests in a jsdom environment which now also supports
web crypto polyfills.

Since ESM support in Jest is currently still limited, it requires a
commonJS build for browser environments, see the discussion in
#616 for all the details.

Co-authored-by: Christoph Tavan <dev@tavan.de>
ctavan added a commit that referenced this pull request Jul 29, 2022
Jest runs browser tests in a jsdom environment which now also supports
web crypto polyfills.

Since ESM support in Jest is currently still limited, it requires a
commonJS build for browser environments, see the discussion in
#616 for all the details.

Co-authored-by: Christoph Tavan <dev@tavan.de>
ctavan added a commit that referenced this pull request Jul 29, 2022
Jest runs browser tests in a jsdom environment which now also supports
web crypto polyfills.

Since ESM support in Jest is currently still limited, it requires a
commonJS build for browser environments, see the discussion in
#616 for all the details.

Co-authored-by: Christoph Tavan <dev@tavan.de>
ctavan added a commit that referenced this pull request Jul 29, 2022
Jest runs browser tests in a jsdom environment which now also supports
web crypto polyfills.

Since ESM support in Jest is currently still limited, it requires a
commonJS build for browser environments, see the discussion in
#616 for all the details.

Co-authored-by: Christoph Tavan <dev@tavan.de>
ctavan added a commit that referenced this pull request Aug 3, 2022
Jest runs browser tests in a jsdom environment which now also supports
web crypto polyfills.

Since ESM support in Jest is currently still limited, it requires a
commonJS build for browser environments, see the discussion in
#616 for all the details.

Co-authored-by: Christoph Tavan <dev@tavan.de>
@ctavan ctavan closed this in #642 Aug 3, 2022
ctavan added a commit that referenced this pull request Aug 3, 2022
Jest runs browser tests in a jsdom environment which now also supports
web crypto polyfills.

Since ESM support in Jest is currently still limited, it requires a
commonJS build for browser environments, see the discussion in
#616 for all the details.

Co-authored-by: Christoph Tavan <dev@tavan.de>
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@SimenB SimenB deleted the patch-1 branch August 3, 2022 10:12
@ctavan
Copy link
Member

ctavan commented Aug 5, 2022

I have released uuid@9.0.0-beta.0 that contains this patch. Please try it out and let me know if it fixes the Jest interoperability (to be tested with jest@29.0.0-alpha.1).

@monsieurnebo
Copy link

I have released uuid@9.0.0-beta.0 that contains this patch. Please try it out and let me know if it fixes the Jest interoperability (to be tested with jest@29.0.0-alpha.1).

I just gave it a try and it seems to fix the issue on my side. Looking forward for the stable release(s).

Thanks for your work 💪

@SimenB
Copy link
Contributor Author

SimenB commented Aug 5, 2022

@ctavan can confirm it allows us to remove the moduleMapper config at work as well 👍

@SimenB
Copy link
Contributor Author

SimenB commented Aug 25, 2022

FWIW, Jest 29 is released (which means jest-environment-jsdom@29 is out, which comes with jsdom@20)

@anandhsonu1994
Copy link

@SimenB @ctavan, When can we expect the release?

@SimenB
Copy link
Contributor Author

SimenB commented Sep 1, 2022

I have no idea, Jest 29 has been out for a week

@monsieurnebo
Copy link

monsieurnebo commented Sep 5, 2022

FWIW, Jest 29 is released (which means jest-environment-jsdom@29 is out, which comes with jsdom@20)

Thanks for the info. Awaiting uuid v9 now then :)

@ctavan
Copy link
Member

ctavan commented Sep 5, 2022

uuid@9.0.0 has now been released to npm. Please create a new bug in case this doesn't fix your issue.

@uuidjs uuidjs locked as resolved and limited conversation to collaborators Sep 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.