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

feat: add ability to specify additional npm rebuild args #881

Merged
merged 2 commits into from
Nov 5, 2016
Merged

feat: add ability to specify additional npm rebuild args #881

merged 2 commits into from
Nov 5, 2016

Conversation

mmckegg
Copy link
Contributor

@mmckegg mmckegg commented Nov 5, 2016

I am building an electron app that uses native dependencies targeting multiple platforms. Prebuild binaries are available for all of them.

However when npmSkipBuildFromSource is enabled, prebuild downloads the binary for the installed node version instead of the electron version. This can be easily handled by specifying a --abi=ELECTRON_ABI_VERSION argument, but electron-builder does not allow this.

This PR adds support for specifying any number of additional npm rebuild arguments to allow working around issues outlined above.

if (additionalArgs) {
if (Array.isArray(additionalArgs)) {
additionalArgs.forEach(arg => npmExecArgs.push(arg))
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Please put else on next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -54,6 +54,14 @@ export function spawnNpmProduction(command: string, appDir: string, forceBuildFr
npmExecArgs.push("--build-from-source")
}

if (additionalArgs) {
if (Array.isArray(additionalArgs)) {
additionalArgs.forEach(arg => npmExecArgs.push(arg))
Copy link
Member

Choose a reason for hiding this comment

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

Please use spread operator here: npmExecArgs.push(...additionalArgs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -237,11 +237,12 @@ export class Packager implements BuildInfo {
}
else {
const forceBuildFromSource = this.devMetadata.build.npmSkipBuildFromSource !== true
const additionalArgs = this.devMetadata.build.npmArgs
Copy link
Member

Choose a reason for hiding this comment

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

Var is used only in the one else branch, please inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@develar
Copy link
Member

develar commented Nov 5, 2016

Probably we should always pass --abi=ELECTRON_ABI_VERSION?

@develar develar added the feature label Nov 5, 2016
@mmckegg
Copy link
Contributor Author

mmckegg commented Nov 5, 2016

@develar yes, that would work too. But ELECTRON_ABI_VERSION needs to be computed, that text was just a placeholder. This can be obtained by executing electron --abi.

@develar
Copy link
Member

develar commented Nov 5, 2016

This can be obtained by executing electron --abi

I suppose it will be more good solution — to avoid explicit configuration. To simplify developer life.

@mmckegg
Copy link
Contributor Author

mmckegg commented Nov 5, 2016

I've addressed those review notes.

I think that electron-builder should handle setting the --abi for the user, but still think this is worth merging to handle any other strange cases like this.

@develar Shall I create an issue?

@develar develar merged commit eec5b32 into electron-userland:master Nov 5, 2016
@develar
Copy link
Member

develar commented Nov 5, 2016

PR about abi version is welcome :) Thanks for spotting it.

develar pushed a commit that referenced this pull request Nov 5, 2016
Whoops, when fixing code as per review in #881, accidentally broke the code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants