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

Remove redundant electron prebuilds? #602

Closed
vweevers opened this issue Mar 24, 2019 · 5 comments · Fixed by #608
Closed

Remove redundant electron prebuilds? #602

vweevers opened this issue Mar 24, 2019 · 5 comments · Fixed by #608
Labels
question Support or open question(s) semver-patch Bug fixes that are backward compatible
Milestone

Comments

@vweevers
Copy link
Member

I noticed that node-napi.node and electron-napi.node are equal, at least for Linux. So we can save on npm package size by only including node-napi.node (doesn't matter to node-gyp-build, it will treat that as a "runtime agnostic" prebuild).

Under what conditions do they not match? 🤔

@vweevers vweevers added the question Support or open question(s) label Mar 24, 2019
@vweevers vweevers added this to the 5.0.0 milestone Mar 24, 2019
@ralphtheninja
Copy link
Member

Hmm, that's a good question. cc @mafintosh

@vweevers vweevers modified the milestones: 5.0.0, 5.1.0 Mar 25, 2019
@vweevers
Copy link
Member Author

FYI

npm notice package: [email protected]
npm notice === Tarball Contents ===
...
npm notice 389.3kB prebuilds/android-arm/node-napi.node
npm notice 394.4kB prebuilds/android-arm64/node-napi.node
npm notice 295.3kB prebuilds/darwin-x64/electron-napi.node
npm notice 295.3kB prebuilds/darwin-x64/node-napi.node
npm notice 388.5kB prebuilds/linux-arm/node-napi.node
npm notice 406.8kB prebuilds/linux-arm64/node-napi.node
npm notice 407.1kB prebuilds/linux-x64/electron-napi.node
npm notice 407.1kB prebuilds/linux-x64/node-napi.node
npm notice 489.5kB prebuilds/win32-x64/electron-napi.node
npm notice 501.2kB prebuilds/win32-x64/node-napi.node
npm notice 249B    scripts/cross-compile
npm notice === Tarball Details ===
npm notice name:          leveldown
npm notice version:       5.0.0
npm notice package size:  2.0 MB
npm notice unpacked size: 5.2 MB
npm notice shasum:        44bb893346c2791b3b50276171a5421ecd83f2a9
npm notice integrity:     sha512-MZhLrJEanXmaM[...]3eF8Pw1QYBSDg==
npm notice total files:   195

@vweevers
Copy link
Member Author

See nodejs/node-addon-api#269 - it seems only Windows needs a separate Electron prebuild.

@vweevers
Copy link
Member Author

vweevers commented Mar 29, 2019

Preliminary support matrix:

OS Electron electron-napi.node node-napi.node
Windows 10 x64 3.1.8 ✔️ ❌ (nodejs/node-addon-api/issues/269)
Windows 10 x64 4.1.2 ❌ (cryptic error) ✔️
Linux x64 3.1.8 identical build > TBD
Linux x64 4.1.2 identical build > TBD
MacOS x64 3.1.8 ✔️ ✔️
MacOS x64 4.1.2 ✔️ ✔️

@vweevers
Copy link
Member Author

Given these results, I think our best bet is to ditch electron-napi.node for all platforms, and to recommend Windows users to either use --build-from-source or upgrade to Electron 4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Support or open question(s) semver-patch Bug fixes that are backward compatible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants