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

[12.0.0][regression] error when bundling the cjs version #2411

Closed
AviVahl opened this issue Nov 4, 2021 · 7 comments · Fixed by #2412
Closed

[12.0.0][regression] error when bundling the cjs version #2411

AviVahl opened this issue Nov 4, 2021 · 7 comments · Fixed by #2412

Comments

@AviVahl
Copy link
Contributor

AviVahl commented Nov 4, 2021

Describe the bug
webpack picks either the commonjs or the esm version, depending on the file requesting it and how it does that (require call or dynamic import from a commonjs file).
esm bundles successfully (esm file importing sinon).
cjs, on the other end (cjs file requiring sinon), gets the following bundling error:

WARNING in ./node_modules/sinon/pkg/sinon.cjs 1:529-530
Critical dependency: require function is used in a way in which dependencies cannot be statically extracted

ERROR in ./node_modules/sinon/pkg/sinon.cjs 1:538-545
Cannot statically analyse 'require(…, …)' in line 1

webpack compiled with 1 error and 1 warning

To Reproduce
Steps to reproduce the behavior:

mkdir sinon-bundled
cd sinon-bundled
npm init -y
npm i sinon webpack webpack-cli -D
mkdir src
echo "require('sinon');" > src/index.js
npx webpack --mode development

Expected behavior
Both cjs and esm versions should be bundling-compatible.

Context (please complete the following information):

  • Library version: 12.0.0
  • Environment: Fedora 35
@fatso83
Copy link
Contributor

fatso83 commented Nov 4, 2021

Ah, I looked into this now, and your example actually breaks on every version from 9 and up. 8 is the latest version that works:

$ npm i sinon@8 -D
$ npx webpack --mode development
asset main.js 1.18 MiB [emitted] (name: main)
runtime modules 221 bytes 1 module
modules by path ./node_modules/ 1.1 MiB
  modules by path ./node_modules/@sinonjs/ 58.5 KiB
    modules by path ./node_modules/@sinonjs/samsam/lib/ 42.1 KiB 26 modules
    modules by path ./node_modules/@sinonjs/commons/lib/ 8.91 KiB 18 modules
    ./node_modules/@sinonjs/formatio/lib/formatio.js 7.51 KiB [built] [code generated]
  modules by path ./node_modules/sinon/lib/ 110 KiB
    modules by path ./node_modules/sinon/lib/sinon/*.js 90.3 KiB 19 modules
    modules by path ./node_modules/sinon/lib/sinon/util/ 18.4 KiB 17 modules
    ./node_modules/sinon/lib/sinon.js 1.31 KiB [built] [code generated]
  6 modules
./src/index.js 46 bytes [built] [code generated]
webpack 5.61.0 compiled successfully in 521 ms

@AviVahl
Copy link
Contributor Author

AviVahl commented Nov 4, 2021

previous major worked. It just required the util npm polyfill package to be installed if I remember correctly. WIll check.

@AviVahl
Copy link
Contributor Author

AviVahl commented Nov 4, 2021

Yep, works:

mkdir sinon-bundled
cd sinon-bundled
npm init -y
npm i sinon@11 webpack webpack-cli util -D
mkdir src
echo "require('sinon');" > src/index.js
npx webpack --mode development

(notice the sinon@11 and util in the npm i arguments)

This was found in a real project currently using the previous major: wixplosives/file-services#523

@perrin4869
Copy link
Contributor

#2412 Fixes the issue for me and all tests are passing! @AviVahl can you confirm?
Not sure what to do about pkg/sinon-no-sourcemaps.cjs... I'd kinda vote for removing it since I really don't see a use for it, but you never know who's using it ><;;

@fatso83
Copy link
Contributor

fatso83 commented Nov 4, 2021

@AviVahl I just ran your commands on all the versions from 8.11 to 9.2.4 and it broke on 9.2.3

$ pbpaste
8.1.1
9.0.0
9.0.1
9.0.2
9.0.3
9.1.0
9.2.0
9.2.1
9.2.2
9.2.3
9.2.4

$ for v in $(pbpaste|dos2unix); do  npm i sinon@$(/bin/echo -e $v) -D && npx webpack --mode development || break; done

$ jq .version node_modules/sinon/package.json
"9.2.3"

This added util.inspect, which not be be filled by your util, as you added later, so checks out.


Will await your confirmation that the fix supplied by perrin checks out before merging.

@AviVahl
Copy link
Contributor Author

AviVahl commented Nov 4, 2021

I've tested the fix locally by changing node_modules/sinon/package.json -> exports -> require to point to "./lib/sinon.js". works on my end once I add util package (like the previous major). 👍

(webpack@4 used to polyfill util OOTB; webpack@5 stopped automated polyfilling of node apis)

@fatso83
Copy link
Contributor

fatso83 commented Nov 4, 2021

Published as 12.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants