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(version): add support for monorepos #1557

Closed
wants to merge 2 commits into from

Conversation

dhruvdutt
Copy link

The .git folder lookup in version command is based on localPrefix which is not the right location always for monorepos where .git folder is at a different location and not there npm version command is executed.

Fixes by doing the right lookup that will support .git in monorepos.

References

Fixes #1082, #9111

lib/version.js Outdated Show resolved Hide resolved
Co-authored-by: Jordan Harband <ljharb@gmail.com>
TomJKing added a commit to nationalarchives/tdr-generated-graphql that referenced this pull request Aug 12, 2020
npm version will not commit version updates if the package.json is in a different directory to .git directory: npm/npm#9111

There is an open PR to add this feature to npm version: npm/cli#1557

In the interim include git commands to commit the changes to the branch
@ruyadorno ruyadorno added Enhancement new feature or improvement pr: needs tests requires tests before merging Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes labels Aug 14, 2020
@dhruvdutt
Copy link
Author

@ruyadorno Can you please guide on fixing tests?

@ruyadorno
Copy link
Collaborator

hi @dhruvdutt my understanding here is that tests are failing because they were relying on the fact that in the nested folder from which the commands are run (in the context of these tests) there is no .git folder, that would cause the command to fail, which is the expected behavior being asserted in these current failing tests: https://github.com/npm/cli/blob/latest/test/tap/version-lifecycle.js#L62

Since the git rev-parse command will work from any sub-directory of a git repo, the proposed change from this PR will result in these assertions failing - since the statGitFolder call will now succeed instead of failing as it was the case before, because it's reading rev-parse info from the repo itself 😁

Now some important considerations: It's worth noting that we're heading towards end of beta for npm7 and this command has been entirely rewrote for the new version and this logic now lives on: https://github.com/npm/libnpmversion - so maybe a more long-lasting contribution would be to validate how it's currently behaving in the v7 beta and contribute the fixes to libnpmversion instead? (IMO this fix is actually very important in v7 since that's the first version which will ship with support to workspaces but I'm not sure if it's still behaves the same in v7)

@darcyclarke darcyclarke added pr: needs tests requires tests before merging and removed Enhancement new feature or improvement pr: needs tests requires tests before merging labels Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: needs tests requires tests before merging Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QUESTION] npm version doesn't create git commit/tag in subpackages
4 participants