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

chore(deps): update dependency chai to v5 #2070

Closed

Conversation

pichlermarc
Copy link
Member

update chai to v5, adapt tests.

Replaces #2013

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Merging #2070 (f3b020e) into main (dfb2dff) will decrease coverage by 0.34%.
Report is 25 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2070      +/-   ##
==========================================
- Coverage   90.97%   90.63%   -0.34%     
==========================================
  Files         146      146              
  Lines        7492     7488       -4     
  Branches     1502     1494       -8     
==========================================
- Hits         6816     6787      -29     
- Misses        676      701      +25     

see 8 files with indirect coverage changes

@pichlermarc pichlermarc marked this pull request as ready for review April 2, 2024 07:40
@pichlermarc pichlermarc requested a review from a team April 2, 2024 07:40
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but I have a Q.

https://github.com/chaijs/chai/releases/tag/v5.0.0

  • chai@5 is now ESM only. IIUC, the commonjs() plugin in "web-test-runner.config.mjs" is converting the test/foo.test.ts from TypeScript to CommonJS to ESM before executing the test, so ... using an ESM-only test lib works. TIL.
  • chai@5 release notes say it drops support for Node.js less than 18. Currently browser tests are run with node v16. Is this a potential problem?

@pichlermarc
Copy link
Member Author

Approving, but I have a Q.

https://github.com/chaijs/chai/releases/tag/v5.0.0

* chai@5 is now ESM only. IIUC, the `commonjs()` plugin in "web-test-runner.config.mjs" is converting the test/foo.test.ts from TypeScript to CommonJS to ESM before executing the test, so ... using an ESM-only test lib works. TIL.

* chai@5 release notes say it drops support for Node.js less than 18. Currently browser tests are run with node v16.  Is this a potential problem?

Oh yes, you're right. That's a problem, we shouldn't update this as it'll likely bite us later.

Unfortunately we'd need to update webpack to use Node 18. We've done it in core so I know that it can work. 🤔 I think the only reason we use webpack in this repo is because our tests use karma which is actually deprecated. So another option would be to change all our web tests to use @web/test-runner which uses rollup (see #1816, #2005), we'd be able to kick both webpack and karma and upgrade to Node 18 in the workflow

Unfortunately I don't have deep knowledge of the tooling and no time to dig into it so I cannot help the author along in #2005 at the moment. I'll close this PR until we find time to sort it out.

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

Successfully merging this pull request may close these issues.

5 participants