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

Add explicit export for ./* #2413

Merged
merged 1 commit into from Jan 28, 2022
Merged

Add explicit export for ./* #2413

merged 1 commit into from Jan 28, 2022

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Nov 4, 2021

Purpose

Add an explicit export entry for package.json to fix the following error when running karma:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './package.json' is not defined by "exports" in node_modules/sinon/package.json

Background

nodejs/node#33460

There is a lot of discussion around whether declaring package.json as "public API" via exports is a good or bad idea. My take is simply that we need to expose this as a workaround before node team decide what to do, otherwise it breaks many tools that relies on package.json.

How to verify

Dependabot update sinon 12.0.0 failed on CI and manually patching package.json tested and working locally.

E.g. https://github.com/ntkme/github-buttons/runs/4099992660?check_suite_focus=true

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

@fatso83
Copy link
Contributor

fatso83 commented Nov 4, 2021

This is great, so thanks for this. But to avoid issues of this kind I would like to not rely on after-the-fact bug reports, but have our own little set of integration tests of sorts to catch these things when they happen. So could you instruct me of what a minimal setup for reproduction of this issue would be?

Something a la

mkdir karma-integration && cd karma-integration && npm init -y
npm i sinon karma
cat > karma.conf.js << EOF
// ??
EOF

Having that, we could run this set of integration tests before every release.

@ntkme
Copy link
Contributor Author

ntkme commented Nov 4, 2021

Let me try come with a small test that can repro the issue.

@ntkme
Copy link
Contributor Author

ntkme commented Nov 5, 2021

Did a little bit more digging and the issue is caused by this line in karma-sinon-chai:

https://github.com/kmees/karma-sinon-chai/blob/c0663bcf150a4f28b0225f91d878a33d2ce8992d/index.js#L28

@ntkme
Copy link
Contributor Author

ntkme commented Nov 5, 2021

Opened PR against karma-sinon-chai: kevicency/karma-sinon-chai#64

If it get fixed in karma-sinon-chai, I think this PR might not be necessary. Let's see.

@perrin4869
Copy link
Contributor

Hey dude, in case you are interested, I setup some karma based testing in this repo:
https://github.com/dotcore64/react-stay-scrolled/blob/master/karma.conf.js
It uses chai, sinon etc

@fatso83
Copy link
Contributor

fatso83 commented Jan 19, 2022

@perrin4869 That was a 404 when I checked.

@perrin4869
Copy link
Contributor

Sorry, in the meantime it changed to cjs: https://github.com/dotcore64/react-stay-scrolled/blob/master/karma.conf.cjs

@fatso83
Copy link
Contributor

fatso83 commented Jan 20, 2022

Since kmees/karma-sinon-chai seems quite dead, I assume that fix is not landing. I had quick glance the karma config and the karma-runner/karma-mocha, but I could not immediately figure out how to create a headless test (though I saw the HeadlessChrome bits in the config), so I would appreciate if someone just put the bits together.

@ntkme ntkme changed the title Add explicit export for package.json Add explicit export for ./* Jan 26, 2022
@ntkme
Copy link
Contributor Author

ntkme commented Jan 26, 2022

Chai.js has recently merged a change that fixes this issue by adding export for ./*. This allows all files in this package to be required in traditional way. I just updated this PR to do the same.

@fatso83
Copy link
Contributor

fatso83 commented Jan 27, 2022

This feels a bit scary to merge without any tests of typical consumers ... I just remember that every time we have touched the exports field someone has gotten bitten 😨

We could of course just ship a patch version and see what noise comes up. If it's breaking in some way, we could revert or fix it.

@ntkme
Copy link
Contributor Author

ntkme commented Jan 27, 2022

The rationale behind this:

  • For node <12, exports in package.json is ignored. All files in the package can be loaded by referencing package_name/relative/path/in/package.js.
  • For node >=12, . inside exports defines the top level exports. ./* allows all files to be loaded similar to node <12. - The only catch being in node <12 relative/path maybe resolve to relative/path/index.js, but with new exports mapping "./*": "./*" any file named index.js can only be resolved explicitly as relative/path/index.js, but not relative/path. A quick file search shows that sinon does not use index.js as filename at all so that it should not be an issue. In the end, this is just restoring the original behavior we had until node 12.

If you like we can wait a week or two and see if any issue arise on chai.js regarding the change on their side.

@fatso83 fatso83 merged commit a00f14a into sinonjs:master Jan 28, 2022
@fatso83
Copy link
Contributor

fatso83 commented Jan 28, 2022

Thanks for the explanation! Will release a new major version anyway after upgrading the fake-timers package.

@ntkme ntkme deleted the patch-1 branch January 28, 2022 15:31
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 this pull request may close these issues.

None yet

3 participants