-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: update Taskfile install:frontend:deps, check only .bin directory… #4698
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
base: v3-alpha
Are you sure you want to change the base?
Conversation
… in node_modules for checksum's, to stop `npm link` causing an infinite loop with certain paths and improve performance.
WalkthroughChanged the Taskfile template to narrow the generated-files pattern for the frontend install task from Changes
Sequence Diagram(s)(Skipped — change is a single-file configuration tweak and documentation update that does not alter runtime control flow.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai the code walkthrough has an incorect statement. should be Which is the actual change. |
🧩 Analysis chain🏁 Script executed: Length of output: 44 🏁 Script executed: Length of output: 575 @Snider: You're absolutely correct! Thank you for catching that error. Looking at the actual diff, the change was indeed from Would you like me to regenerate the summary with the correct information? ✅ Actions performedSummary regeneration triggered. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 37 🏁 Script executed: Length of output: 1318 @Snider, you're absolutely correct! Thank you for catching that inaccuracy. The actual change was indeed from I appreciate the correction! |
yes please @coderabbitai |
|
@Snider: I'll regenerate the summary now with the corrected information. The walkthrough will reflect the accurate change from ✅ Actions performedSummary regeneration triggered. |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/UNRELEASED_CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
v3/UNRELEASED_CHANGELOG.md
[grammar] ~3-~3: Use a hyphen to join words.
Context: ...ng dependency change detection for front end builds made with wails3 init. ## ...
(QB_NEW_EN_HYPHEN)
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...## Fixed - Fixes an issue with the front end directory if a local npm package is ...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
🔇 Additional comments (1)
v3/UNRELEASED_CHANGELOG.md (1)
5-9: Changelog entries are clear and accurately document the changes.The "Changed" and "Fixed" sections clearly align with the PR objectives: narrowing the Taskfile pattern to
node_modules/.bin/*for improved re-build performance, and resolving the local npm package installation issue. The entries are well-structured and provide sufficient context for users.
leaanthony
left a comment
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.
Did you mean to review the comments? We need to leave them in so new contributors know where to add things.
Oh, that's just me being a noob trying to be helpful, misreading a failing CI check ^.^ Was I meant to leave the file alone? |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/UNRELEASED_CHANGELOG.md (1)
29-29: Consider adding issue reference for traceability.Per the changelog guidelines, issue/PR numbers should be referenced when applicable. The Fixed entry describes a bug fix that resolves #4697; consider adding that reference for improved traceability and context.
Apply this diff to add the issue reference:
-- Fixes an issue with the front-end directory if a local npm package is installed `npm i ~/my-lib` +- Fixes an issue with the front-end directory if a local npm package is installed `npm i ~/my-lib` (#4697)
|
|
Why do we only want to check the bin directory for module changes? |
Because using the level above makes Task checksum the entire node_modules folder to check for changes, which is slower the more you have, and totally stops compiling at all when using |
|
Actually this should probably just be package.json and forget the rest 😅 |



Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (4697)
Type of change
Bug fix (non-breaking change which fixes an issue)
How Has This Been Tested?
wails builds my project / acts the same as npm install. Has the path change to the build template been tested? No, but the change has.
https://discord.com/channels/1042734330029547630/1065600636294549514/1435715828384268348
You would need to grab an npm module, install it in a project as a local file path in a project made with wails3 init, and notice how it doesn't segfault Goland.
Summary by CodeRabbit