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

Shouldn't @mapbox/node-pre-gyp be under devDependencies? #364

Closed
Nantris opened this issue Nov 16, 2022 · 10 comments
Closed

Shouldn't @mapbox/node-pre-gyp be under devDependencies? #364

Nantris opened this issue Nov 16, 2022 · 10 comments

Comments

@Nantris
Copy link

Nantris commented Nov 16, 2022

Template questions N/A.

We're using version 0.29.1 and I noticed @mapbox/node-pre-gyp is bundled with our app - but as I understand it, that will never be necessary for end users. Is it included where it is to facilitate some part of the build process?

Specifically I noticed that electron-builder includes it in the bundled application since it's marked as a dependency.

Thanks again @ranisalt for all the amazing work!

@ranisalt
Copy link
Owner

Not really, because it is used when installing the package, to select and download prebuilt binaries when available (see the install script)

@Nantris
Copy link
Author

Nantris commented Nov 29, 2022

Couldn't that still be a devDependency though? Because doesn't the install only need to occur on the developer's machine?

@ranisalt
Copy link
Owner

Couldn't that still be a devDependency though? Because doesn't the install only need to occur on the developer's machine?

It runs every time the package is installed or updated. You are right that it will not be needed after installation, but npm/yarn do not support i.e. installDependencies to get around that.

An alternative solution would be creating a package for each architecture supported, and use a trick with optionalDependencies, os and cpu in package.json to selectively install it. That, however, is much more complicated to maintain. This is what @node-rs/argon2 does.

@Nantris
Copy link
Author

Nantris commented Nov 29, 2022

It runs every time the package is installed or updated. You are right that it will not be needed after installation, but npm/yarn do not support i.e. installDependencies to get around that.

I think I follow that, but maybe I don't, because my concern isn't that it sticks around in our node_modules on the development machine - which makes sense to me and I don't see any problem there.

My concern is that it ends up bundled into our application and distributed to our application's end users, who should never need node-pre-gyp, because the compiled binary is already downloaded/built and then bundled.

Wouldn't it still be available for the install script to run properly if it was under devDependencies?

@ranisalt
Copy link
Owner

Wouldn't it still be available for the install script to run properly if it was under devDependencies?

It does not, devDependencies are only installed for the current package i.e. for the package.json in the directory where npm/yarn are currently being run. For example, dev dependencies will not get installed for any dependencies of your project, even if you are "developing" it - package manages will not distinguish between "development machine" and "end user machine".

Here's a detailed explanation of each dependency type.

@Nantris
Copy link
Author

Nantris commented Nov 30, 2022

Ah yes, that makes sense. Thanks very much for the explanation!

@Nantris Nantris closed this as completed Nov 30, 2022
@Nantris
Copy link
Author

Nantris commented Mar 29, 2023

Version 30 and up seem to require @mapbox/node-pre-gyp even in the final bundled app. Any idea why that might be @ranisalt?

Unpacked it's around 400kb. I'd love to avoid including it if possible. I'm not sure why it might be needed since node-gyp won't run in production.

@ranisalt
Copy link
Owner

It will be required as long as npm and yarn don't support install-time-only dependencies.

Considering how large it is, it might be worth it to simply bundle all prebuilt binaries into the package, as they account for just under 300 KB which is less than the dependency needed to pick and download the correct one...

@Nantris
Copy link
Author

Nantris commented Mar 29, 2023

Thanks as always for your reply!

It will be required as long as npm and yarn don't support install-time-only dependencies.

That make sense as far as why it needs to be listed as a dependency - but manually excluding it from our bundle used to be fine and now it crashes argon2 at runtime, so I was wondering if something changed there?

Considering how large it is, it might be worth it to simply bundle all prebuilt binaries into the package, as they account for just under 300 KB which is less than the dependency needed to pick and download the correct one...

That makes a lot of sense. My only concern would be that perhaps the dependencies are more compressible than the binaries.

@ranisalt
Copy link
Owner

That make sense as far as why it needs to be listed as a dependency - but manually excluding it from our bundle used to be fine and now it crashes argon2 at runtime, so I was wondering if something changed there?

Ah! I understand. Can you provide a minimal reproducible example that I can investigate?

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

No branches or pull requests

2 participants