Conversation
erickzhao
changed the title
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
erickzhao
dismissed
georgexu99’s stale review
All comments were addressed in subsequent commits.
| "node-fetch": "^2.6.7", | ||
| "rechoir": "^0.8.0", | ||
| "semver": "^7.2.1", | ||
| "source-map-support": "^0.5.13", |
There was a problem hiding this comment.
aside: I don't think we're using this package ✂️
| case 'arm': | ||
| return 'armv6hl'; |
There was a problem hiding this comment.
note: as noted in electron/packager#1830, the TargetArch type from Packager 18 (which is extended by the ForgeArch type used in rpmArch) used to evaluate to just string, so arm was a legal value even though it wasn't explicitly supported by Packager. Since the RPM maker handled arm differently, though, this is technically a breaking change (you could get Node 22.22.0 for the archv7l arch on Linux and run Forge 8 on something like a 32-bit Raspberry Pi, which will produce process.arch === 'arm' 🤷🏼♂️)
Assuming we don't care about armv6hl, I wonder if we should have rpmArch return armv7hl or aarch64 when nodeArch === 'arm' and update the test accordingly, so that this maker at least provides some valid input to electron-installer-redhat.
There was a problem hiding this comment.
Hmm, I was considering just dropping support for arch/platform combos that weren't supported by Packager in this PR 😅
There was a problem hiding this comment.
Fair enough, as long as we call it out as a breaking change in the commit message. In that case, should we have the default branch throw an unsupported arch/platform combo error and refuse to proceed, or do you think it's ok to just keep using the provided arch even if it's not supported as we do now?
| // The entrypoint can be defined under `exports` or `main` in package.json now! | ||
| const main = pj.exports ?? pj.main; |
There was a problem hiding this comment.
nit: all subpackages have only the exports field set now, so we should be good to ditch the pj.main fallback here:
| // The entrypoint can be defined under `exports` or `main` in package.json now! | |
| const main = pj.exports ?? pj.main; | |
| const main = pj.exports; |
packages/api/core/src/api/package.ts
Outdated
| export function serialHooks<T extends (...args: any[]) => any>( | ||
| hooks: T[] = [], | ||
| ): [T] { | ||
| return [ | ||
| async function (opts: Parameters<T>[0]): Promise<void> { | ||
| for (const hook of hooks) { | ||
| await hook(opts); | ||
| } | ||
| } as T, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
suggestion: Packager exports an identical serialHooks function that we can use directly:
| export function serialHooks<T extends (...args: any[]) => any>( | |
| hooks: T[] = [], | |
| ): [T] { | |
| return [ | |
| async function (opts: Parameters<T>[0]): Promise<void> { | |
| for (const hook of hooks) { | |
| await hook(opts); | |
| } | |
| } as T, | |
| ]; | |
| } | |
| import { | |
| // ... | |
| serialHooks | |
| } from '@electron/packager'; |
There was a problem hiding this comment.
Oops sorry. I think I actually implemented this in Forge first before upstreaming the changes to Packager haha. c6a65db
There was a problem hiding this comment.
note: now that importSearchRaw always does a dynamic import, we should always return null if the module has no default export so that we don't get an error down the line if the user-provided file exists and can be correctly imported, but has only named exports by mistake.
For instance, the code below would cause this function to return [Module: null prototype] { afterCopyHook: [Function: afterCopyHook] }, and Packager would then attempt to call the entire module object as a function:
// user-provided-after-copy-hook.js
export function afterCopyHook() {
// ^ user error: missing `default`
}| return result?.default || null; |
Maybe we could add a test case for this scenario?
packages/api/core/src/api/package.ts
Outdated
There was a problem hiding this comment.
note: still on the importSearch issue, we're not filtering out null return values here, so Packager will attempt to call null as a function and throw an error.
This isn't directly related to this PR, though, as it was also possible to get null when using require if the user-provided file had no module.exports.
4 participants
BREAKING CHANGE: this package now requires Node.js 22 and ESM. All
@electron/packages are bumped to their latest major versions.Closes #3623
Closes #3684
Closes #4059
enginesto Node 22@types/nodedependencies@electron/packages to their latest major versionstype: modulein package.jsonts-nodewithtsxdistutilsand Python 3.11lodashwithlodash-esSplit off PRs:
.nvmrcfile for CI #4081ts-nodewithtsx#4085pre-pushhook #4117Future improvements:
jitifor the configuration loading