Fix issue with peerDependencies coming from sinon and chai

[deleonn]

Description

The dependencies sinon-as-promised, sinon-chai and chai were causing issues due to peerDependencies now being installed by default and said packages pointing to incompatible dependency versions between them. The current problem is that simply updating them is breaking the npm i for node@12 and node@14. In order to prevent this behavior eslint was updated to version 8, which was asking for a different version of acorn.

How to Test

1. Pull the branch: bug/sc-104915/error-when-installing-dependencies-in-particle
2. Update your local node version to v16.x and npm to version to v8.x. Use nvm use 16 or equivalent.
3. Verify your updates: node -v && npm -v. Make sure you are running npm@8.6.x or higher. This particular version is causing the package-lock.json to get modified after doing an install
4. Install dependencies: npm i or npm run reinstall
5. Verify the package-lock.json does not have changes
6. Run the tests npm run test

Outcome
All tests should pass. npm i should run without issues.

Related / Discussions

• https://app.shortcut.com/particle/story/104915/error-when-installing-dependencies-in-particle-api-js-with-npm-8-11-0

Completeness

• PR opened 🎉
• Testing instructions have been provided
• Tracking story is up to date (tasks are marked complete, state is "ready for review")
• Docs have been updated
• Branch is rebased against target (typically main)

[busticated]

@deleonn

Make sure you are running npm@8.11.x or higher.

can you explain why this is required?

[busticated]

@deleonn testing instructions should include verifying the lockfile does not have any changes after a fresh install / reinstall - can you update those 🙏 👍

[joegoggins]

I needed to do some research to understand the background of this PR. I found this helpful: https://stackoverflow.com/questions/66239691/what-does-npm-install-legacy-peer-deps-do-exactly-when-is-it-recommended-wh

The PR title makes it sound like chai and sinon are the underlying thing that is throwing this error. Is that because we're running older versions of these testing tools?

Specifically, npm outdated shows:

Package                          Current  Wanted  Latest  
chai                               3.5.0   3.5.0   4.3.6
chai-as-promised                   5.3.0   5.3.0   7.1.1
mocha                              2.5.3   2.5.3  10.0.0
sinon                              7.2.5   7.5.0  14.0.0
sinon-chai                         3.3.0   3.7.0   3.7.0

If we were running newer versions of the tools that are throwing this error, would this problem go away?
If so, it seems like that would be a better route to addressing this error rather than using a cryptic legacy-peer-deps flag that locks us into behavior from earlier versions of npm (>=4, <=6).

[deleonn]

@joegoggins starting from npm@7, peerDependencies are now installed by default, which is what the legacy-peer-deps flag prevents.

I tried to upgrade all the related sinon and chai dependencies that are causing issues, and also removed some that are not needed after the update (one of them is explicitly asking for a very old sinon@1 version), but the CI started to fail for node@12 and node@14 (see https://app.circleci.com/pipelines/github/particle-iot/particle-api-js/31/workflows/05ed37e1-5cb6-4a4c-a68e-2bc5915dfb97/jobs/89) so in the meantime I decided to stick with legacy-peer-deps while I figure how to properly update the dependencies.

[deleonn]

@busticated @joegoggins seems like the missing piece was adding acorn as a dependency to the package.json (just like the CI was saying). The latest update should fix the problems we saw by updating sinon and chai instead of using the legacy-peer-deps flag.

[busticated]

@deleonn you'll need to dig deeper i think - run npm ls acorn locally on this branch and observe:

└─┬ eslint@5.16.0
  └─┬ espree@5.0.1
    ├─┬ acorn-jsx@5.1.0
    │ └── acorn@8.7.1 deduped invalid: "^6.0.0 || ^7.0.0" from node_modules/acorn-jsx
    └── acorn@6.4.0

then trace back those dependencies:

eslint@5.16.0 uses espree@5.0.1: https://github.com/eslint/eslint/blob/v5.16.0/package.json#L47
espree@5.0.1 uses acorn@6.4.0 and acorn-jsx@5.1.0: https://github.com/eslint/espree/blob/v5.0.1/package.json#L21-L22
acorn-jsx@5.1.0 uses peerDependencies which do not accept acorn@8.x: https://github.com/acornjs/acorn-jsx/blob/5.1.0/package.json#L22

bottom-line: we're on a very old version of eslint (latest is v8.17.0, we're on v5.16.0). even upgrading to eslint@7, ultimately gets us acorn-jsx@5.3.1 which has a compatible peerDependencies range: https://github.com/acornjs/acorn-jsx/blob/5.3.1/package.json#L22 - i would look at upgrading eslint to the v8 line or v7 if there are known-blockers (see breaking changes). since espree@7.3.1 declares acorn as a direct dependency (source), that should satisfy acorn-jsx's peerDependencies range.

[busticated]

i tested using npm versions 8.5.5, 8.11.0, and 8.12.1 - looks good 👍

@deleonn for posterity, please update this PR's testing instructions per 👉 #144 (comment)

[joegoggins]

Nice work @deleonn , I'm stoked you were able to find a way to upgrade deps rather than use that funky flag. 👍