Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

node package improvements #15748

Merged
merged 2 commits into from Oct 8, 2019
Merged

node package improvements #15748

merged 2 commits into from Oct 8, 2019

Conversation

springmeyer
Copy link
Contributor

@springmeyer springmeyer commented Oct 3, 2019

Currently the node bindings include, in the package.json, a fallback to source compiles:

"install": "node-pre-gyp install --fallback-to-build=false || make node",

This is non-desirable for 3 reasons:

  • It's confusing: when a binary is not available, first a user is presented with lots of lines of errors before the source compile starts, eroding confidence in the build, even if the source compile later succeeds.
  • The source compile is unlikely to work. Users running npm install expect a binary and don't likely have the correct development environment to source compile. So in the rare circumstances where this fallback is triggered we should fail fast rather than try to fallback.
  • Keeping support for source compile fallbacks leads to an enormous package tarball. Currently the package for @mapbox/mapbox-gl-native on npm is 244 MB on disk due to the large ./vendor folder (`166 MB itself)

So, my proposal is to:

  • Expect that users who run npm install will only get a binary, if available
  • Stop falling back to a source compile
  • Document in the developing.md that to compile mbgl source a user needs to:
    • clone from git
    • run make node
  • Add an .npmignore to radically trim the size of the uploaded package on npm

/cc @mapbox/gl-native @mapbox/static-apis @kkaefer

@springmeyer
Copy link
Contributor Author

With these changes the size on disk is now 172 kb

platform/macos
platform/qt
platform/node/symbol-list
platform/node/version-script
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option I considered to reduce package size while avoiding having to declare all these things is to move the package.json into platform/node/ and do the packaging from there. But that would be more distruptive of a change, so I did not take that route.

Also, I wish there was such a thing as .npminclude but it does not exist: npm/npm#13171

@@ -49,7 +49,7 @@
"node": ">=6"
},
"scripts": {
"install": "node-pre-gyp install --fallback-to-build=false || make node",
"install": "node-pre-gyp install --fallback-to-build=false",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change means that npm install will still work and fetch a binary. But if a binary is not found the install will exit instead of calling make node.


To create an Xcode project and use a GUI debugger in the case of a crash, run `make xnode`.
```bash
make node
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears that make xnode does not exist anymore so I removed reference to it. Not sure when the xnode target was removed /cc @friedbunny @tmpsantos


```
npm run test-suite
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this since i can't find any sign of its existence anymore

@@ -9,7 +9,7 @@ Requires a modern C++ runtime that supports C++14.
By default, installs binaries. On these platforms no additional dependencies are needed.

- 64 bit macOS or 64 bit Linux
- Node.js v4.x _(note: v5+ is known to have issues)_
- Node.js v10.x
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are only supporting + building with node v10 currently, so this drifted out of date

@springmeyer
Copy link
Contributor Author

springmeyer commented Oct 7, 2019

👋 @brendan-ward @petrsloup @klokan - I noticed you are maintainers of modules that depend on @mapbox/mapbox-gl-native (based on https://www.npmjs.com/package/@mapbox/mapbox-gl-native). Are any of you critically depending on the ability to run npm install @mapbox/mapbox-gl-native and have it fallback to a source compile? This PR removes that functionality which means that going forward:

  • npm install @mapbox/mapbox-gl-native will only install binaries
  • currently binaries are available for node v8, v10, and v12
  • if binaries are not available then the build still stop
  • if you want to source compile for a version of node that does not support binaries you'd need to clone the github repo and run make node

Let me know if you have any concerns.

@brendan-ward
Copy link

@springmeyer thanks for reaching out!

I believe that we will be fine with just binary versions; I was only relying on building from source until the binaries became consistently available (~4.0?). I 👍 the idea of reducing the package size as we go to some lengths in our Dockerfile to reduce some of these build dependencies.

The only place where I've been trying to build from source is in trying to setup a new alpine-based Docker image, which has not been successful yet due to GL issues, and for that I totally expect to clone the repo and build from source as part of the build chain - NOT via npm install.

So - I think your assumptions above are valid for our use case.

@springmeyer
Copy link
Contributor Author

Great, thanks so much for the confirmation + feedback @brendan-ward. I'm also interested in alpine. I've had success with other projects on alpine, but have not attempted mbgl-native yet. Feel free to cc me on tickets where you are exploring that - I would be interested to take a look.

@petrsloup
Copy link

@springmeyer Thanks for letting us know! We don't have any issue with this change -- we usually use mapbox-gl-native in a controlled docker environment (debian-based) and the prebuilt binaries are just fine.

@springmeyer
Copy link
Contributor Author

@petrsloup excellent, thanks for confirming. That's a relief. Glad to know that the pre-built binaries work well in the docker setup too.

Copy link

@ian29 ian29 left a comment

Choose a reason for hiding this comment

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

thank you for putting to this together @springmeyer

@springmeyer springmeyer merged commit d0281e3 into master Oct 8, 2019
@springmeyer springmeyer deleted the node-package-fixes branch October 8, 2019 23:27
@springmeyer springmeyer mentioned this pull request Oct 8, 2019
@friedbunny friedbunny added build Node.js node-mapbox-gl-native labels Oct 15, 2019
acalcutt added a commit to WifiDB/maplibre-gl-native that referenced this pull request Oct 23, 2022
Put back node-pre-gyp and node-pre-gyp-github to allow binaries to continue to be supported. if node-pre-gyp is not able to find a binary, cmake-js is run as a fallback.

this somewhat relates to mapbox/mapbox-gl-native#15748 , where support for building was removed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build Node.js node-mapbox-gl-native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants