-
Notifications
You must be signed in to change notification settings - Fork 2
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
Improve node_modules resolver #219
Conversation
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.
Code looks good and works as advertised 👍 🚢
@@ -67,30 +67,30 @@ export const jsMiddleware = async ({ | |||
try { | |||
// Normalized path starting with slash | |||
const path = posix.normalize(req.path); | |||
const params = new URLSearchParams(req.query as Record<string, 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.
I wanted to flag the warnings above. They don't seem critical and I don't see them as a merge blocker but wanted to make sure you were aware.
(For some reason GitHub wouldn't let me comment directly on them.
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.
Thanks 👍
I'm not sure whether at this point there is a good opportunity to factor out part of this function. For now I think I'll leave it as-is but I'll keep my eyes out for refactoring opportunities 👀
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.
Cool, works for me 👍
This PR finishes some edge-ish cases with resolution:
For #1, here is an example:
Before this PR, PT only looks at
node_modules/preact
. This is bad for two reasons, one is thatnode_modules/preact
isn't guaranteed to exist unless we have it as a direct dep, and also, it will always return 10.1.0, even though when importing from some-library, it should return 8.0.0.This PR fixes that, so it will find the nearest node_modules to the importer that has the requested package.
Both of these changes are necessary for pnpm support. I tested it out by using pnpm with https://github.com/cloudfour/pleasantest-example-modal, linking my local PT in, and making sure the tess pass.
I added tests for #1 but not for #2. #2 can be tested manually:
Testing pnpm
In this repo:
Clone https://github.com/cloudfour/pleasantest-example-modal
Closes #176