fix: add special case for sharp@0.33.0 optional dependencies #372
Conversation
styfle
changed the title
|
Looks like So we might need to change the repo to use npm or at least this specific test needs npm. @lovell is that expected? |
|
Adding |
|
yarn v1 has been in maintenance mode for almost 4 years and does not properly support |
Ethan-Arrowood
force-pushed
the
issue-371
branch
from
b011392
to
4d6d3e5
Compare
|
@styfle Whatcha think about this PR? Merge it? |
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
|
Great to see some progress on this, thank you. The sharp-related change adds 10 lines whereas the majority of this PR appears to relate to replacing yarn v1 with the latest npm. If clean version control history is of interest then may I suggest a split into a PR for the package manager upgrade and a PR to support |
|
Even if we split it into 2 PRs, it will still have the same problem because we can't run tests without installing This should be wrapping up soon though, there was a bug in jest causing some confusion and I think we have it resolved. |
| @@ -13,6 +13,9 @@ console.log('created directory ' + tmpdir); | |||
|
|
|||
| // These are tests known to fail so we skip them | |||
| const ignoreCategories = new Set([ | |||
| 'Generator function.sent Meta Property', | |||
| 'Class and Property Decorators', | |||
| 'throw expressions', | |||
| ]); | |||
There was a problem hiding this comment.
We are skipping these tests now because they currently fail.
At first I thought this must be a regression since the tests were passing on main (head is 285c930). However, the tests were always failing and jest was silently succeeding. Regenerating the lockfile, implicitly upgraded jest from 27.4.5 to 27.5.1 (due to the ^ prefix) and this included a fix that is now surfacing the error.
I also converted this file from jest to node:test just to confirm that it also fails on main.
Since these tests are Stage 2 features, likely no one has noticed they don't work because no JS runtime implements them.
|
Looks like we're running into this npm bug now 🤦 |
|
🎉 This PR is included in version 0.26.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
There's a need to skip some integration tests for Windows, this PR moves the logic for skipping to the test setup itself rather than altering the tests and dependencies as we were doing in the `prepare-install` step. The pre-install logic was also deleting the `package-lock.json`, I'm not 100% clear on what the purpose was, but seems like it may have been done accidentally given the context of the [diff here](#372 (comment)). With the previous logic, CI started to fail on Windows because the `package-lock.json` was removed before install, causing us to install a later version of dependency. Instead of removing that line of code in the `prepare-install` file, just trying this approach which feels a little more straightforward. --------- Co-authored-by: Nathan Rajlich <n@n8.io>
@vercel/nftdoes not pick up platform-specific binaries forsharp#371Additionally:
yarnwithnpm(sharp requires it)@ffmpeg-installer/ffmpeg,oracledb,sharpto versions which support Apple Siliconmicrotime-node-gyptest on Apple SiliconI also skipped a few ECMAScript Next tests because they currently fail.
At first I thought this must be a regression since the tests were passing on main (head is 285c930). However, the tests were always failing and jest was silently succeeding. Regenerating the lockfile, implicitly upgraded jest from
27.4.5to27.5.1(due to the^prefix) and this included a fix that is now surfacing the error.I also converted this file from
jesttonode:testjust to confirm that it also fails on main.Since these tests are Stage 2 features, likely no one has noticed they don't work because no JS runtime implements them.