-
Notifications
You must be signed in to change notification settings - Fork 10
Conversation
core/garment/src/garment.ts
Outdated
@@ -919,7 +919,7 @@ async function garmentFromWorkspace( | |||
|
|||
if ( | |||
subscription.type === 'glob' && | |||
event.path.startsWith(subscription.input.rootDir) | |||
event.path.startsWith(Path.normalize(subscription.input.rootDir)) |
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.
@beshanoe I'm not really comfortable with this change. Though it does work, it adds extra file input-level subscriptions to allSubscriptionsCached
:
Before fix:
{
"type": "file",
"level": "input",
"path": "C:\\Users\\stephan.noel\\farfetch-repos\\ui-box\\babel.config.js",
"targetPath": "C:/Users/stephan.noel/farfetch-repos/ui-box/packages/accordion/src/context/focus-provider/FocusProvider.js"
}, {
"type": "glob",
"level": "input",
"input": {
"rootDir": "C:/Users/stephan.noel/farfetch-repos/ui-box/packages/typography/src",
"include": ["**/*.js?(x)"],
"exclude": ["**/*.test.js", "**/__fixtures__", "**/__mocks__"]
}
}, {
"type": "file",
"level": "input",
"path": "C:\\Users\\stephan.noel\\farfetch-repos\\ui-box\\babel.config.js",
"targetPath": "C:/Users/stephan.noel/farfetch-repos/ui-box/packages/typography/src/index.js"
}
After fix:
{
"type": "file",
"level": "input",
"path": "C:\\Users\\stephan.noel\\farfetch-repos\\ui-box\\babel.config.js",
"targetPath": "C:/Users/stephan.noel/farfetch-repos/ui-box/packages/accordion/src/context/focus-provider/FocusProvider.js"
}, {
"type": "file",
"level": "input",
"path": "C:\\Users\\stephan.noel\\farfetch-repos\\ui-box\\babel.config.js",
"targetPath": "C:\\Users\\stephan.noel\\farfetch-repos\\ui-box\\packages\\accordion\\src\\components\\AccordionButton.js"
}, {
"type": "glob",
"level": "input",
"input": {
"rootDir": "C:/Users/stephan.noel/farfetch-repos/ui-box/packages/typography/src",
"include": ["**/*.js?(x)"],
"exclude": ["**/*.test.js", "**/__fixtures__", "**/__mocks__"]
}
}, {
"type": "file",
"level": "input",
"path": "C:\\Users\\stephan.noel\\farfetch-repos\\ui-box\\babel.config.js",
"targetPath": "C:/Users/stephan.noel/farfetch-repos/ui-box/packages/typography/src/index.js"
}
We could try the approach of changing what's stored in the cache itself, but this would include changing the createdInput, rootDir (probably in getActionGraph) and targetPath to be OS-specific. Not sure if the TypeScript compiler approach of dealing with only one kind of path internally is a good strategy?
TLDR;
This "fix" should work for now, but it has some unwanted effects. Taking the approach of platform-specific paths in the cache itself may imply changes in a lot of places. Which do you think is preferable?
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 didnt understand how does it changes allSubscriptionCached? It seems to only affect changesBySubscriptions. We can also probably normalize the path before adding it to the subscription? 🤔
Is the path unix-style inside the watcher event?
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.
Changed the changesBySubscription
entry to normalize as suggested, but using normalizePath
. It's now working without extra entries in allSubscriptionsCached
.
It is failing in build due to an outdated github action. I can try looking into it and after it should be ready to merge.
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.
can we use normalizePath
on this line too? Just to make it look unified. Or they work differently in this case?
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.
Sure thing. They basically do the opposite, so I can just use normalize-path
on event.path
instead of using Path.normalize
on subscription.input.rootDir
. Changed, what do you think?
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.
Looks good to me
87024f2
to
9dc2ef2
Compare
- v1.4.2 used the add-path command which was deprecated and disabled due to a security vulnerability - This was causing consumer workflows to fail
9dc2ef2
to
72280b4
Compare
72280b4
to
96c9375
Compare
Description
Fixes watch mode not working on windows.
Fixes #25
How Has This Been Tested?
Tried change in node_modules of the project where error was occurring and debugged the effects.
This can still be tested on a Mac to ensure nothing was broken.
Checklist:
Disclaimer
By sending us your contributions, you are agreeing that your contribution is made subject to the terms of our Contributor Ownership Statement