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

fix: restore installation of @node-rs/crc32 optional deps #984

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

achou11
Copy link
Member

@achou11 achou11 commented Jan 7, 2025

4f33bd0 updated the lockfile to remove the native binaries produced by @node-rs/crc32, which prevents me from locally running anything that touches yauzl-promise (which uses that module). This has happened in the past and I honestly don't really understand what the expected behavior should be or why this happens in general.

The modules in question are marked as optional deps in the published package, but not sure how npm decides to add/remove these from the lockfile. And if they're considered optional, not sure why @node-rs/crc32 complains if the relevant binaries are missing at runtime (i.e. not truly optional by definition).

Explicitly marking the module as a dependency for us seems to restore the lockfile so that the native binaries are included, but my guess is that this isn't the desired solution to the problem.

@achou11 achou11 requested a review from gmaclennan January 7, 2025 21:12
@awana-lockfile-bot
Copy link

package-lock.json changes

Click to toggle table visibility
Name Status Previous Current
@emnapi/core ADDED - 1.3.1
@emnapi/runtime ADDED - 1.3.1
@emnapi/wasi-threads ADDED - 1.0.1
@napi-rs/wasm-runtime ADDED - 0.2.6
@node-rs/crc32-android-arm-eabi ADDED - 1.10.6
@node-rs/crc32-android-arm64 ADDED - 1.10.6
@node-rs/crc32-darwin-arm64 ADDED - 1.10.6
@node-rs/crc32-darwin-x64 ADDED - 1.10.6
@node-rs/crc32-freebsd-x64 ADDED - 1.10.6
@node-rs/crc32-linux-arm-gnueabihf ADDED - 1.10.6
@node-rs/crc32-linux-arm64-gnu ADDED - 1.10.6
@node-rs/crc32-linux-arm64-musl ADDED - 1.10.6
@node-rs/crc32-linux-x64-gnu UPDATED 1.10.3 1.10.6
@node-rs/crc32-linux-x64-musl UPDATED 1.10.3 1.10.6
@node-rs/crc32-wasm32-wasi ADDED - 1.10.6
@node-rs/crc32-win32-arm64-msvc ADDED - 1.10.6
@node-rs/crc32-win32-ia32-msvc ADDED - 1.10.6
@node-rs/crc32-win32-x64-msvc ADDED - 1.10.6
@node-rs/crc32 UPDATED 1.10.3 1.10.6
@tybys/wasm-util ADDED - 0.9.0

@achou11
Copy link
Member Author

achou11 commented Jan 7, 2025

Ah, a hint!

https://github.com/napi-rs/node-rs/blob/11a9a1f37f9b42899071789f1232556420600983/packages/crc32/index.js#L358-L361

I've definitely tried the node_modules reinstallation approach, but that doesn't seem to work for me (given that the lockfile doesn't specify any of the binaries as deps)

@achou11 achou11 force-pushed the ac/restore-noders-crc32-optional-deps branch from ce9b6d3 to 28a77aa Compare January 22, 2025 17:06
Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

looks ok, however can you check the next APK we build with this change, to ensure we aren't unnecessarily shipping a bunch of platform binaries that are included in this change?

@achou11
Copy link
Member Author

achou11 commented Jan 22, 2025

looks ok, however can you check the next APK we build with this change, to ensure we aren't unnecessarily shipping a bunch of platform binaries that are included in this change?

yeah i don't want to run into something like this so my plan is to pack this branch and try this out locally for the mobile app before merging this

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

Successfully merging this pull request may close these issues.

2 participants