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

install: de-duplicate code in downloadNodeLib() function #965

Closed
wants to merge 2 commits into from

Conversation

pmed
Copy link
Contributor

@pmed pmed commented Jun 20, 2016

Modified the result of processRelease() to return uniform names libUrl,
libPath for Windows in objects named as target platforms (ia32 and x64)

This allows to loop over the target platforms in downloadNodeLib() and to use
the universal names instead of two almost identical code parts with libUrl32,
libPath32 and libUrl64, libPath64 names.

Modified the result of processRelease() to return uniform names `libUrl`,
`libPath` for Windows in objects named as target platforms (`ia32` and `x64`)

This allows to loop over the target platforms in downloadNodeLib() and to use
the universal names instead of two almost identical code parts with `libUrl32`,
`libPath32` and `libUrl64`, `libPath64` names.
done(new Error(res.statusCode + ' status code downloading 32-bit ' + release.name + '.lib'))
return
var archs = ['ia32', 'x64']
var async = archs.length
Copy link
Member

Choose a reason for hiding this comment

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

best avoid the async keyword for future-proofing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are many variables named async in the code above, I just used the same name.

@rvagg
Copy link
Member

rvagg commented Jun 27, 2016

I don't mind this refactor although I consider the changes in process-release to be breaking because that module is usable outside of node-gyp (I've used it myself that way). The change is good, but I'd rather hold this off till v4, which we're close to because of the NVM_* env var removals we need to do.

@pmed
Copy link
Contributor Author

pmed commented Jun 27, 2016

As I see process-release is not exported from the node-gyp in any way. Thus these changes in the process-release are only details of implementation. And I think no other external projects can rely on internals of node-gyp.

@rvagg
Copy link
Member

rvagg commented Jul 7, 2016

require('node-gyp/lib/process-release')

but anyway, we can break stuff in v4 so whatever

@rvagg
Copy link
Member

rvagg commented Jun 21, 2019

sorry this got stale @pmed, still want to pursue it? if so, rebase to current master and let's take a look

@pmed
Copy link
Contributor Author

pmed commented Jun 21, 2019

Hi @rvagg

I've rebased it on the master.

rvagg pushed a commit that referenced this pull request Jun 22, 2019
Modified the result of processRelease() to return uniform names `libUrl`,
`libPath` for Windows in objects named as target platforms (`ia32` and `x64`)

This allows to loop over the target platforms in downloadNodeLib() and to use
the universal names instead of two almost identical code parts with `libUrl32`,
`libPath32` and `libUrl64`, `libPath64` names.

PR-URL: #965
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Jun 22, 2019

thanks @pmed, landed in 03683f0
it's pretty surprising the duplication lasted this long isn't it? good work.

@rvagg rvagg closed this Jun 22, 2019
@rvagg rvagg mentioned this pull request Jun 26, 2019
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.

None yet

2 participants