build: upgrade to Lerna 9 and Yarn 4#4009
Conversation
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
|
tests won't pass until lerna v9 reaches 7 day release maturity |
|
Also there seems to be a lockfile update required: |
| '..', | ||
| '.links', | ||
| ); | ||
|
|
There was a problem hiding this comment.
to resolve linking issues, we must custom retrieve each workspace's path and link them with yarn 4 syntax
| '.links', | ||
| ); | ||
|
|
||
| const getWorkspacePath = (packageName: string): string => { |
There was a problem hiding this comment.
I hate how much extra work we have to do with Yarn 4 to make linking our packages is... but if it works, it works. 🤷
There was a problem hiding this comment.
yep i hate this too
| const mainOut = path.join(appPath, '.webpack/main'); | ||
|
|
||
| beforeAll(async () => { | ||
| await fs.promises.mkdir(appPath, { recursive: true }); |
There was a problem hiding this comment.
Can you explain a bit what the root cause of the test failures was that caused this refactor?
There was a problem hiding this comment.
the assetrelocator patch test creates a webpack project and tests that webpack assets are in the right place once built- the test ran npm install inside a fixture directory inside the forge monorepo, which caused yarn to lock it down and fail the test as it required native module builds (native-hello-world).
This beforeAll moves the fixture to a temp directory outside the monorepo so it can run without yarn's configuration policies.
There was a problem hiding this comment.
I don't love bypassing the yarn policies in yarnrc.yml, can we do the same lockfile trick here and use a pre-generated one? (Similar to what you did below)
|
@SocketSecurity ignore npm/init-package-json@8.2.2 This is a package maintained by npm and there's no suspicious activity on the package itself. |
There was a problem hiding this comment.
This is unsafe, we shouldn't do it
There was a problem hiding this comment.
This is unsafe, we shouldn't do it
package.json
Outdated
There was a problem hiding this comment.
Does preversion still work in Yarn 4? I don't see it documented anymore: https://yarnpkg.com/cli/version
There was a problem hiding this comment.
Can we remove these flags now that the expectation is that the developer runs this update script locally?
a77319b to
ba56129
Compare
- Remove unnecessary packages from npmPreapprovedPackages in .yarnrc.yml - Change node engine requirement from ^16.4.0 to >=16.4.0 Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
- Add macos-alias to npmPreapprovedPackages to enable native volume.node compilation on macOS - Replace file: protocol with link: protocol for test fixtures to avoid platform-dependent hashes - This should fix DMG maker tests and eliminate hash mismatch errors in CI Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
The macos-alias package contains native modules that need to be rebuilt for the current Node.js version in CI. This was causing test failures on macOS runners when running tests that use the DMG maker.
…neration script The script is meant to be run locally by developers who don't have immutable installs enabled by default. Removing these overrides makes the script cleaner and doesn't disable security features unnecessarily.
ba56129 to
976ca66
Compare
MarshallOfSound
left a comment
There was a problem hiding this comment.
Looking solid, left some clean up notes, a neat trick, and one ask RE lockfile stuff
| '.links', | ||
| ); | ||
|
|
||
| const getWorkspacePath = (packageName: string): string => { |
There was a problem hiding this comment.
This whole thing can be replaced with a sick oneliner -->
yarn workspace @electron-forge/package-name exec pwd| const mainOut = path.join(appPath, '.webpack/main'); | ||
|
|
||
| beforeAll(async () => { | ||
| await fs.promises.mkdir(appPath, { recursive: true }); |
There was a problem hiding this comment.
I don't love bypassing the yarn policies in yarnrc.yml, can we do the same lockfile trick here and use a pre-generated one? (Similar to what you did below)
| "@bitdisaster/exe-icon-extractor": { | ||
| "built": true | ||
| }, | ||
| "electron-winstaller": { | ||
| "built": true | ||
| }, |
There was a problem hiding this comment.
@erickzhao can we take action items somewhere to make these not require building. Seems strange to me
There was a problem hiding this comment.
Yeah, I made of note of in Slack but I'll create a separate GitHub issue for it as well.
There was a problem hiding this comment.
|
#4009 (comment) I think the test specifically uses |
|
@georgexu99 @MarshallOfSound @erickzhao hello , #4026 Some bad things have happened after this merge. |
This PR upgrades Yarn to v4, and Lerna to v9.
This sets the stage for configuring trusted publishing on npm.
Yarn upgrades
Yarn Berry comes with a few important upgrades:
Disables lifecycle scripts to minimize chance of supply chain security attacks (with allowlist)
Adds cooldown time for dependencies to reduce chance of supply chain security attacks (set to 1 week)
Built-in yarn patch command means we can get rid of patch-package
Yarn version is now on a per-workspace basis instead of installed globally
Lerna v9
Workarounds