-
Notifications
You must be signed in to change notification settings - Fork 921
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
Support mounting within node_modules directory #3134
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/Ebq5CQEdsPuWUbYYKpTUsQ8xfQV8 |
da8d35c
to
fc9a58c
Compare
snowpack/src/config.ts
Outdated
...ALWAYS_EXCLUDE, | ||
// Always ignore the final build directory. | ||
`${config.buildOptions.out}/**`, | ||
// If you've mounted your current working directory, ignore the node_modules directory. |
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 could see this accidentally including node_modules in a multirepo setup (e.g. if we mount ./packages/foo
, we probably don't want ./packages/foo/node_modules
included).
Ideally, I think we'd still ignore node_modules
but only the ones nested under the mount directory. Maybe something like this would do the trick:
const files = (await new fdir()
.withFullPaths()
// Note: exclude() only matches directories, and not files. However, the cost
// of false positives here is minor, so do this as a quick check to possibly
// skip scanning into entire folder trees.
- .exclude((_, dirPath) => foundExcludeMatch(dirPath))
+ .exclude((_, dirPath) => foundExcludeMatch(path.relative(mountKey, dirPath)))
.crawl(mountKey)
.withPromise()) as string[];
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.
ah right, we're probably getting this protection for free right now.
I really wanted to avoid making a change to every exclude call, but you're right that this is probably the most clear expression of what we're trying to accomplish here. Mount anything, but never travel down into a nested node_modules within a mount (since those are seen as "third-party" to whatever was just mounted).
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.
// if this is mounted:
/Fred/Code/myapp/node_modules/astro/internal": "/internal"
/Fred/Code/myapp/node_modules/astro/internal/node_modules/...
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ should not be picked up by our automatic "node_modules" exclusion.
^^^^^^^^^^^^^^ anything inside of this directory should be ignored.
// "any node_modules that is explictly mounted should not be ignored. otherwise, ignore it."
fc9a58c
to
218b099
Compare
@natemoo-re is going to take this PR over while I'm out! |
test/build/config-mount/package.json
Outdated
@@ -29,9 +30,14 @@ | |||
"url": "/j", | |||
"static": true, | |||
"resolve": false | |||
} | |||
}, | |||
"../../../node_modules/explicit-test-pkg": "/dep-explicit" |
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.
👍🏻
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.
LGTM
a9daa91
to
28c9d98
Compare
28c9d98
to
3430bf0
Compare
Changes
node_modules
exclude to only kick in when no mounts are defined. This was the original intended use-case that this support was added for.Testing
Docs