fix: bundling compatibility with webpack@5

AviVahl commented 1 year ago

Purpose (TL;DR)

Ensure sinon bundles nicely for browsers without any errors or warnings. https://github.com/sinonjs/sinon/issues/2411 re-fixed (after "exports" were added I think).

Background

When using webpack v5 to bundle cjs code that calls require('sinon'), it would have defaulted to "exports->require" and fail with multiple node API errors/warnings (util, timers, etc.).

This patch ensures that anyone who bundles sinon with a bundler that respects "exports" gets the (browser-compatible) esm version.

Tested on both webpack v5 and v4. should be noted that v4 worked even without this patch, as it automatically injected polyfills. webpack@5 no longer does so.

Solution

The esm version bundles without any errors/warnings, so make sure bundlers pick that one up.

How to verify

Use the same steps to reproduce in https://github.com/sinonjs/sinon/issues/2411 Modify the node_modules with this patch and rerun npx webpack --mode development

Checklist for author

[x] npm run lint passes
[x] References to standard library functions are cached.

fatso83 commented 1 year ago

Thanks, seems good! Needs a test run, of course :-)

AviVahl commented 1 year ago

Looks like browserify doesn't like the esm in package.json->browser. I'll revert that one and leave the one in the exports. All should be well then.

AviVahl commented 1 year ago

Reverted. Tests should (hopefully) now pass.

On a related note, please consider dropping browserify support... it's pretty outdated in 2023. Might be better to test widely-used bundlers, like: webpack, rollup, esbuild, etc.

AviVahl commented 1 year ago

@fatso83 heya, any chance we can rerun ci on the smaller change?

codecov[bot] commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (05f05ac) 96.00% compared to head (c649a14) 96.00%.

:exclamation: Current head c649a14 differs from pull request most recent head 6e3f36e. Consider uploading reports for the commit 6e3f36e to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #2519 +/- ## ======================================= Coverage 96.00% 96.00% ======================================= Files 40 40 Lines 1900 1900 ======================================= Hits 1824 1824 Misses 76 76 ``` | Flag | Coverage Δ | | |---|---|---| | unit | `96.00% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sinonjs#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

AviVahl commented 1 year ago

Now passes on CI. :) Would appreciate if this could land main, as it would allow us to remove workarounds to the bundling issues in several repositories.

fatso83 commented 1 year ago

Ironically, this seems to have broken MUI's webpack setup: https://github.com/mui/material-ui/pull/37430#issuecomment-1601845962 👀

AviVahl commented 1 year ago

Yeah, webpack now resolves the esm version, so this config: https://github.com/mui/material-ui/blob/master/test/regressions/webpack.config.js#LL26C1-L29C8 probably needs to now say process: 'process/browser.js'

They might even be able to remove the ProvidePlugin completely if sinon was the only place needing this polyfill.

sinonjs / sinon