Fixed CWD 'node_modules' check for namespaced packages #26
Conversation
index.js
Outdated
| path.basename(path.join.apply(path, [CWD].concat( | ||
| splitName.map(function () { | ||
| return '..' | ||
| })))) === 'node_modules') |
There was a problem hiding this comment.
Thanks for writing this! I find the condition a bit difficult to read and it would be nice to consolidate the === 'node_modules' check into one place so it's not repeated. What about something like this?
var splitName = (process.env.npm_package_name || '').split('/')
var parentDirs = CWD.split(path.sep).slice(-splitName.length - 1, -1)
var isDependency = parentDirs.indexOf('node_modules') !== -1(That is, check the directory names – from immediate parent to the parent indicated by the number of /s in the package name – for node_modules.)
If you have time to update this PR I'll get it merged ASAP!
There was a problem hiding this comment.
It looks to me like with your changes we don't need to keep track of splitName anymore. Your isDependency check is based off of splitting the the CWD apart instead of the package_name.
|
Sorry that review took so long! Went on a camping trip. |
| var isDependency = path.basename(path.dirname(CWD)) === 'node_modules' | ||
| // the parent directory is `node_modules` but the package can be nested | ||
| // in multiple parent directories so we split apart the name of the module. | ||
| var parentDirs = CWD.split(path.sep) |
There was a problem hiding this comment.
I was thinking the splitName length check was still useful for only checking the last few relevant directories in CWD (just to be somewhat cautious), but it might be fine to check every ancestor anyway like this. Do you think it matters?
|
Using the CWD as the check is probably the best way to handle this
because I noticed when using pnpm there is another level of abstraction
in the folder structure. Your solution solves that problem. The only
other thing I'd like to push to this repo when I get a chance, is the
ability to specify or autodetect npm, pnpm, or yarn as the executable to
run against.
…------ Original Message ------
From: "Brian Beck" <[email protected]>
To: "exogen/postinstall-build" <[email protected]>
Cc: "jlaramie" <[email protected]>; "Author"
<[email protected]>
Sent: 7/7/2017 6:08:00 PM
Subject: Re: [exogen/postinstall-build] Fixed CWD 'node_modules' check
for namespaced packages (#26)
@exogen commented on this pull request.
--------------------------------------------------------------------------------
In index.js
<#26 (comment)>:
> @@ -177,8 +177,10 @@ function postinstallBuild () { // dependency, we
should always prune. Unfortunately, npm doesn't set // any helpful
environment variables to indicate whether we're being // installed as a
dependency or not. The best we can do is check whether - // the parent
directory is `node_modules`. - var isDependency =
path.basename(path.dirname(CWD)) === 'node_modules' + // the parent
directory is `node_modules` but the package can be nested + // in
multiple parent directories so we split apart the name of the module. +
var parentDirs = CWD.split(path.sep)
I was thinking the splitName length check was still useful for only
checking the last few relevant directories in CWD (just to be somewhat
cautious), but it might be fine to check every ancestor anyway like
this. Do you think it matters?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#26 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAuIJCpqxgLcNq4OVIuljYsgB7WRhrkoks5sLqxAgaJpZM4OJdtW>.
|
|
@jlaramie Sounds fine to me! Might also fix some issues when npm builds in a Regarding detecting different executables, we'll have to do some experimentation – I think attempting to run the equivalent |
|
Yeah the pruning is a good point. With yarn it wouldn't make a
difference but pnpm would probably be a bit harder to support since it
has additional folder wrapping. I actually switched the project I"m
working on to yarn specifically because I wanted to use this
devDependency. This project solves a long standing problem I have with
npm packages that only build on publish, which prevents you from using
it easily when forking.
…------ Original Message ------
From: "Brian Beck" <[email protected]>
To: "exogen/postinstall-build" <[email protected]>
Cc: "jlaramie" <[email protected]>; "Mention"
<[email protected]>
Sent: 7/7/2017 6:18:05 PM
Subject: Re: [exogen/postinstall-build] Fixed CWD 'node_modules' check
for namespaced packages (#26)
@jlaramie <https://github.com/jlaramie> Sounds fine to me! Might also
fix some issues when npm builds in a .staging subdirectory or whatever
newer versions do differently.
Regarding detecting different executables, we'll have to do some
experimentation – I think attempting to run the equivalent prune
command on other user agents could break more than it fixes, but we'll
have to test it. (Since postinstall-build attempts to leave no trace of
the temporary install anyhow, it might not be worthwhile to try other
commands.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAuIJPhGWiHwCwEaWxZe5r9gVJvs7fvEks5sLq6dgaJpZM4OJdtW>.
|
|
@jlaramie Another thing you may want to explore is a migration path to npm v5's new |
|
Oh I am not versed in that new option. I'm glad that they are adding a
native version of compiling after adding for git repos. I think a
solution to that would be to check the main package.json for prepare
statements and also check the version of node to see if it would get
run.
…------ Original Message ------
From: "Brian Beck" <[email protected]>
To: "exogen/postinstall-build" <[email protected]>
Cc: "jlaramie" <[email protected]>; "Mention"
<[email protected]>
Sent: 7/7/2017 6:22:07 PM
Subject: Re: [exogen/postinstall-build] Fixed CWD 'node_modules' check
for namespaced packages (#26)
@jlaramie <https://github.com/jlaramie> Another thing you may want to
explore is a migration path to npm v5's new prepare command handling,
which basically does what postinstall-build set out to accomplish. It
would be neat if postinstall-build users had an easy migration path
where they could start adding prepare and they wouldn't conflict! (Of
course that depends on people upgrading to npm v5)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAuIJN9U1-eVjrB8DKaY_qaKZM3NTyAuks5sLq-PgaJpZM4OJdtW>.
|
|
Now published in 5.0.1, thanks for the PR! |
|
Thank You! Now I just need to do another pull request to update
paginate-this to use the main repo again.
…------ Original Message ------
From: "Brian Beck" <[email protected]>
To: "exogen/postinstall-build" <[email protected]>
Cc: "jlaramie" <[email protected]>; "Mention"
<[email protected]>
Sent: 7/7/2017 6:28:02 PM
Subject: Re: [exogen/postinstall-build] Fixed CWD 'node_modules' check
for namespaced packages (#26)
Now published in 5.0.1, thanks for the PR!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAuIJFZerU9G5sxTlDOjQNP3nBvkHY8Kks5sLrDygaJpZM4OJdtW>.
|
No description provided.