Add getProjectPath() to compute path to parent project #98
Conversation
73e6b6b to
f0adfd8
Compare
|
Looks alright to me. Can we do this without adding the ends-with dependency? |
|
I see two ways here: drop node |
|
Let's drop node 0.12! Anything before 4.x has been dead to me for a while |
|
Can bump a major version to make that drop more obvious. |
|
Ok, done. I've also manually tested installing/uninstalling commitplease which resulted in further changes to |
|
Spoke too soon. Travis tells me I have not accounted for self-install. Fair enough, let me hack a bit more. |
|
Add early opt-out check for self-install, ready for review. |
| var pkg = fs.existsSync(pkgPath) && require(pkgPath) | ||
|
|
||
| if (pkg && pkg.name && pkg.name === 'commitplease') { | ||
| console.log('commitplease: self-install detected, skipping hooks') |
There was a problem hiding this comment.
I was surprised that this was missing, apparently we lost it earlier - I remember writing a similar check originally. After a bit of digging I found the original code: 2021ce3#diff-fcd5628e27bdebc81efd345a905292c3L1
I guess this new implementation using options.projectPath makes more sense, now that we have that in place. I didn't notice anything from the old implementation that would change this one - do you?
There was a problem hiding this comment.
Oh, now I remember: setup.js was reused in both install.js and uninstall.js, I did not fully understand it and split the two files completely, dropping setup.js. That is when I lost the self-install check.
Anyway, on the one hand I still do not fully understand how that setup used to work. On the other hand, current getOptions() -> getProjectPath() logic is clear to me: while constructing the options, find the best candidate for the project path. If that project path contains a package.json with name: commitpealse, then do not copy the hooks. That logic I tested and it works.
|
Let's land it. |
Previously, process.cwd() did the job just fine but with node v8.1.2 (compared to 7.10.0 at least) the output has changed. So, use the following hack: when installing a node package, npm adds it to PATH. Since we know: a) the suffix of that path entry (commitplease/node_modules/.bin) b) the fact that the other project is installing commitplease then find that entry and use its prefix as parent project path Refs: jquery/jquery#3708
- during install, look for `node_modules/commitplease/node_modules/.bin` - during usual trigger, look for `node_modules/.bin`
Commitplease is called by either npm or git which in turn supply different process.cwd() information. Use custom getProjectPath() function to more reliably determine where the project is actually located.
|
Done, I've also written a short release summary: https://github.com/jzaefferer/commitplease/releases/tag/v3.0.0 The |
|
Nice :-) Summary is looking good, helps explain the major version bump. |
Previously, process.cwd() did the job just fine but with node v8.1.2
(compared to 7.10.0 at least) the output has changed. So, use the
following hack: when installing a node package, npm adds it to PATH.
Since we know:
a) the suffix of that path entry (commitplease/node_modules/.bin)
b) the fact that the other project is installing commitplease
then find that entry and use its prefix as parent project path
Refs: jquery/jquery#3708