Skip to content

Consider using npm module name instead of relative path in adapter config generated by commitizen init #469

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

Open
mickdekkers opened this issue Mar 1, 2018 · 6 comments

Comments

@mickdekkers
Copy link

The setup instructions in the readme use the module name of the adapter (i.e. cz-conventional-changelog), but the actual config generated by commitizen init uses a relative path to the module:

let commitizenAdapterConfig = {
config: {
commitizen: {
path: `./node_modules/${adapterNpmName}`
}
}
};

According to the readme:

commitizen.path is resolved via require.resolve and supports

  • npm modules
  • directories relative to process.cwd() containing an index.js file
  • file base names relative to process.cwd() with js extension
  • full relative file names
  • absolute paths.

One downside of using a relative path is that developers cannot use their global commitizen install to create a commit if they have not yet run npm install in the repo, because this results in an error:

Error: Could not resolve /Users/mickdekkers/Projects/foo/node_modules/cz-conventional-changelog.
Cannot find module '/Users/mickdekkers/Projects/foo/node_modules/cz-conventional-changelog'

This change should fix the issue, although I'm not sure whether it would be considered breaking:

let commitizenAdapterConfig = { 
   config: { 
     commitizen: { 
-      path: `./node_modules/${adapterNpmName}`
+      path: adapterNpmName
     } 
   } 
 }; 
@LinusU
Copy link
Contributor

LinusU commented Mar 1, 2018

Yeah, I found this to be quite confusing myself, PR welcome 👍

@jimthedev
Copy link
Member

jimthedev commented Mar 1, 2018

This change would be breaking. With that said I think we can support this by first checking if the path exists and if not then we see if the npm module exists in either the deps or dev deps. If it does then we can install it as a transient dependency first before running commit.

@mickdekkers @LinusU Does that sound right?

@mickdekkers
Copy link
Author

@jimthedev sounds interesting, what do you mean by "install it as a transient dependency"?

@jimthedev
Copy link
Member

@mickdekkers I mean we would run npm install --no-save [email protected]. Effectively we would detect that the npm install hasn't been run yet and would install module. This ensures that the package.json and package lock is not touched but that commitizen could run.

One tricky thing is that npm changed from --save being implied at a certain version of npm so we might have to detect their npm version.

@LinusU
Copy link
Contributor

LinusU commented Mar 2, 2018

We are only talking about the generated config here, right? So I'm not sure if it would be considered breaking?

I also don't think we would need to install any dependencies automatically? Although that would potentially also be nice!

edit:

We are only talking about changing this line, right?

path: `./node_modules/${adapterNpmName}`

@mickdekkers
Copy link
Author

@LinusU yeah, that's my proposed solution. I think you may be right; since it only applies to initialization and doesn't affect existing projects, it shouldn't be a breaking change.

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

No branches or pull requests

3 participants