-
Notifications
You must be signed in to change notification settings - Fork 623
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
fix: NPX apply changelog (npx @changesets/cli version) #1562
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1562 +/- ##
==========================================
- Coverage 81.02% 81.00% -0.02%
==========================================
Files 54 54
Lines 2229 2232 +3
Branches 662 663 +1
==========================================
+ Hits 1806 1808 +2
- Misses 419 420 +1
Partials 4 4 ☔ View full report in Codecov by Sentry. |
🦋 Changeset detectedLatest commit: 3e44749 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
8655d94
to
b8f393c
Compare
b8f393c
to
865f492
Compare
changelogPath = resolveFrom(changesetPath, config.changelog[0]); | ||
} catch { | ||
changelogPath = resolveFrom( | ||
path.join(__dirname, "..", ".."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you are doing here but I don't think we should just blindly~ escape the current directory like this in hope to end up in the correct directory. This alternative cwd
should be passed down as part of the options to this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolveForm does not seem to accept options.
But isn't cwd the issue here as NPX installs the npm package into a npm cache folder somewhere in the home directory or %localappdata% for windows, while the process itself gets executed in a workspace (potentially) far from this cache directory. Resulting in changeset searching in the local node_modules where the process is executed, while it should search in the npm-cache (close to where the changeset/cli cache is located)
Packages located here:
~/.npm/_npx/<hash>/node_modules/@changesets/cli
~/.npm/_npx/<hash>/node_modules/@changesets/apply-release-plan
~/.npm/_npx/<hash>/node_modules/@changesets/git
Executed here:
~/Documents/React/<project>
__dirname: ~/npm/<hash>/node_modules/@changesets/apply-release-plan/index.js
cwd: ~/Documents/React/<project>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I get what you mean. Applied & testing, seems to work :)
0e25c14
to
cf4c8e4
Compare
try { | ||
changelogPath = resolveFrom(changesetPath, config.changelog[0]); | ||
} catch { | ||
changelogPath = resolveFrom(__dirname, config.changelog[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better but this "context dir" should come from options. I feel like we want to resolve this in the context of the executed @changesets/cli
and not in the context of @changesets/apply-release-plan
, even if that ends up resolving to the same location in the majority of cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like 3c786a6 you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this version works for me 👍 Have you tried testing the current state of this PR in "the real world"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't yet since 3c786a6 (Since the __dirname coming from the cli package), before that I did. https://www.npmjs.com/package/@netail/changeset-cli + underlying https://www.npmjs.com/package/@netail/changesets-apply-release-plan
I can check the latest changes tonight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can check the latest changes tonight
I'd appreciate it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, seems to work
ref npx @netail/changeset-cli@2.27.21 version
(https://www.npmjs.com/package/@netail/changeset-cli)
a66d857
to
3c786a6
Compare
…1562) * Resolve module from global node_modules Fixes changesets#566 * fix: NPX apply changelog * fix: feedback * fix: feedback v2 * fix: feedback v3 * Create seven-beans-jump.md * Apply suggestions from code review --------- Co-authored-by: Cefn Hoile <github.com@cefn.com> Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
#1500, but a bit simpler
fixes: #566
fixes: #1426
If you would like to test it yourself;
npx @netail/changeset-cli@2.27.15 version