Conversation
There was a problem hiding this comment.
Code Review
This pull request updates package.json to define supported architectures and enhances the after-pack.cjs script to better manage native package cleanup. Key changes include the introduction of platform aliasing, architecture normalization, and support for unscoped native packages. Review feedback suggests improving regex consistency by using non-capturing groups and optimizing the unscoped package cleanup logic to reduce redundant directory reads.
| '@snazzah': /^davey-(darwin|linux|android|freebsd|win32|wasm32)-(x64|arm64|arm|ia32|arm64-gnu|arm64-musl|x64-gnu|x64-musl|x64-msvc|arm64-msvc|ia32-msvc|arm-eabi|arm-gnueabihf|wasi)/, | ||
| '@lydell': /^node-pty-(darwin|linux|win32)-(x64|arm64)/, | ||
| '@reflink': /^reflink-(darwin|linux|win32)-(x64|arm64|x64-gnu|x64-musl|arm64-gnu|arm64-musl|x64-msvc|arm64-msvc)/, | ||
| '@node-llama-cpp': /^(mac|linux|win)-(arm64|x64|armv7l)(-metal|-cuda|-cuda-ext|-vulkan)?$/, |
There was a problem hiding this comment.
The comment on line 107 states that each regex should have two capture groups (platform and arch). This regex has a third capturing group for the suffix. While the code currently ignores it, it would be cleaner and more consistent with the documentation to make it a non-capturing group.
'@node-llama-cpp': /^(mac|linux|win)-(arm64|x64|armv7l)(?:-metal|-cuda|-cuda-ext|-vulkan)?$/,
| // 2. Unscoped packages (e.g. sqlite-vec-darwin-arm64) | ||
| for (const { pattern } of UNSCOPED_NATIVE_PACKAGES) { | ||
| let entries; | ||
| try { entries = readdirSync(nodeModulesDir); } catch { continue; } | ||
|
|
||
| for (const entry of entries) { | ||
| const match = entry.match(pattern); | ||
| if (!match) continue; | ||
|
|
||
| const pkgPlatform = PLATFORM_ALIASES[match[1]] || match[1]; | ||
| const pkgArch = baseArch(match[2]); | ||
|
|
||
| const isMatch = | ||
| pkgPlatform === platform && | ||
| (pkgArch === arch || pkgArch === 'universal'); | ||
|
|
||
| if (!isMatch) { | ||
| try { | ||
| rmSync(join(nodeModulesDir, entry), { recursive: true, force: true }); | ||
| removed++; | ||
| } catch { /* */ } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This implementation for cleaning unscoped native packages has a couple of areas for improvement:
- Inefficiency:
readdirSync(nodeModulesDir)is called inside the loop overUNSCOPED_NATIVE_PACKAGES. If this list grows, it will read the entirenode_modulesdirectory multiple times, which can be slow. It's more efficient to read the directory once and then iterate through the patterns for each entry. - Unused Property: The
prefixproperty defined for unscoped native packages (line 125) is not used. It could be used as an optimization to quickly filter directory entries.
Here is a suggested refactoring that addresses both points by restructuring the loops and utilizing the prefix property.
// 2. Unscoped packages (e.g. sqlite-vec-darwin-arm64)
let entries;
try {
entries = readdirSync(nodeModulesDir);
} catch {
// Could not read directory, nothing to do.
}
if (entries) {
for (const entry of entries) {
for (const { prefix, pattern } of UNSCOPED_NATIVE_PACKAGES) {
if (prefix && !entry.startsWith(prefix)) {
continue;
}
const match = entry.match(pattern);
if (match) {
const pkgPlatform = PLATFORM_ALIASES[match[1]] || match[1];
const pkgArch = baseArch(match[2]);
const isMatch =
pkgPlatform === platform &&
(pkgArch === arch || pkgArch === 'universal');
if (!isMatch) {
try {
rmSync(join(nodeModulesDir, entry), { recursive: true, force: true });
removed++;
} catch { /* */ }
}
// This entry has been matched and processed, so we can skip to the next entry.
break;
}
}
}
}
There was a problem hiding this comment.
The suggested refactoring in my previous comment is indeed more efficient. By reading the directory once and iterating through the UNSCOPED_NATIVE_PACKAGES list for each entry, you avoid redundant I/O operations. The use of the prefix property further optimizes this by allowing you to skip entries that clearly do not match any known package pattern. I recommend applying this change to improve the performance of the cleanup script.
| // 2. Unscoped packages (e.g. sqlite-vec-darwin-arm64) | |
| for (const { pattern } of UNSCOPED_NATIVE_PACKAGES) { | |
| let entries; | |
| try { entries = readdirSync(nodeModulesDir); } catch { continue; } | |
| for (const entry of entries) { | |
| const match = entry.match(pattern); | |
| if (!match) continue; | |
| const pkgPlatform = PLATFORM_ALIASES[match[1]] || match[1]; | |
| const pkgArch = baseArch(match[2]); | |
| const isMatch = | |
| pkgPlatform === platform && | |
| (pkgArch === arch || pkgArch === 'universal'); | |
| if (!isMatch) { | |
| try { | |
| rmSync(join(nodeModulesDir, entry), { recursive: true, force: true }); | |
| removed++; | |
| } catch { /* */ } | |
| } | |
| } | |
| } | |
| // 2. Unscoped packages (e.g. sqlite-vec-darwin-arm64) | |
| let entries; | |
| try { | |
| entries = readdirSync(nodeModulesDir); | |
| } catch { | |
| entries = []; | |
| } | |
| for (const entry of entries) { | |
| for (const { prefix, pattern } of UNSCOPED_NATIVE_PACKAGES) { | |
| if (prefix && !entry.startsWith(prefix)) continue; | |
| const match = entry.match(pattern); | |
| if (!match) continue; | |
| const pkgPlatform = PLATFORM_ALIASES[match[1]] || match[1]; | |
| const pkgArch = baseArch(match[2]); | |
| const isMatch = | |
| pkgPlatform === platform && | |
| (pkgArch === arch || pkgArch === 'universal'); | |
| if (!isMatch) { | |
| try { | |
| rmSync(join(nodeModulesDir, entry), { recursive: true, force: true }); | |
| removed++; | |
| } catch { /* */ } | |
| } | |
| break; | |
| } | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the post-packaging cleanup script to better manage native dependencies across different platforms and architectures. Key changes include adding supported architecture metadata to package.json, implementing platform name normalization, and expanding the cleanup logic to support additional scoped and unscoped native packages. I have no feedback to provide.
Summary:
Notes:
Validation: