Get the library running again without errors [Breaking change] #482
Conversation
BREAKING CHANGE: no longer offer an es5 version of the module
BREAKING CHANGE: convert npm-check to a pure esm module as many of its dependencies have taken this approach see https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c
| return script.indexOf(moduleName) !== -1; | ||
| return script.includes(moduleName); |
There was a problem hiding this comment.
You can thank xo --fix 😆
| const currentState = await npmCheck({}); | ||
| const packages = await currentState.get('packages'); | ||
| t.truthy(packages.length > 0); | ||
| }); |
There was a problem hiding this comment.
Can we add a test that runs the cli tool as well? As that was the previous test
There was a problem hiding this comment.
On this branch, I've changed the npm test script to:
"test": "npm run lint && ava && ./bin/cli.js || echo Exit Status: $?."So I've added the ava test to the existing test of running cli.js, rather than replacing it.
|
I have no idea why the bot is bumping the engines field since that would be a breaking change 🤦♂️ #481 But could you change the engines field to the following in this PR? And I'll try and disable that in the bot. "node": "^12.20.0 || ^14.13.1 || >=16.0.0"Could you also change the CI to test on exactly those versions? (I think it's much more likely that we accidentally depend on something only available in newer versions of Node, than that Node.js breaks backwards compatibility in a minor version) node-version: [12.20.0, 14.13.1, 16.0.0] |
|
Oh, and could you also remove |
Co-authored-by: Linus Unnebäck <[email protected]>
Co-authored-by: Linus Unnebäck <[email protected]>
Co-authored-by: Linus Unnebäck <[email protected]>
|
Thanks for the great feedback @LinusU. I think I've addressed everything – would you mind taking another look please? It seems that removing the config file didn't quite placate AppVeyor - see https://ci.appveyor.com/project/dylang/npm-check/builds/45424492. It looks like it's reverted to some sort of default configuration. Perhaps we need to delete the project in the AppVeyor UI? |
|
Awesome! Just realised that I'm not actually admin on this repo 😅 @dylang could you remove AppVeyor, and enable GitHub Actions please? |
Background
Last week I used
npm-checkfor the first time in a while. I pulled the code and the test script isn't passing at the moment. The reason for this is that therenovatebot has upgraded a number of packages and quite a few of them (notably some maintained by https://github.com/sindresorhus) have dropped support for CommonJS in favour of ESM.You can see Sindre’s motivations here: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c.
Approach
I think we can safely convert
npm-checkto a pure ESM module too. Most users are likely using the library via the CLI, so it should make no difference to them. For those importing the library, they’ll be able to use an older version ofnpm-checkor do anasync import.I think we can safely drop support for es5. Even if there were some older projects still using es5, CLI users could switch node versions as a workaround and library users could pin to an older version.
This PR:
npm-checkto a pure ESM module (breaking change)lib.xoconfig to its own file so it doesn't take up too much space in thepackage.jsonavabecause ESM support injestis experimental.node14, 16 and 18.The net result is that the library can now be run without errors.
cc @dylang