-
Notifications
You must be signed in to change notification settings - Fork 146
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: support pattern trailers #361
base: main
Are you sure you want to change the base?
fix: support pattern trailers #361
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.
Orca Security Scan Summary
Status | Check | Issues by priority | |
---|---|---|---|
Passed | Secrets | 0 0 0 0 | View in Orca |
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 for posting this, it is definitely a missing feature and the last missing feature before this resolver implementation can be considered complete (given this resolve feature landed in Node.js later itself).
I left a couple of comments. That said I don't think the implementation is complete and likely needs a closer look at the spec. It also needs tests to land.
If you need implementation inspiration, perhaps take a look at rollup/plugins#1549 which was very nicely written.
(a, b) => b.length - a.length | ||
)) { | ||
if (!match.endsWith("/")) continue; | ||
for (const match of Object.keys(matchObj).sort((a, b) => b.length - a.length)) { |
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 sort should be updated to use the order defined by PATTERN_KEY_COMPARE
in https://nodejs.org/dist/latest-v20.x/docs/api/esm.html#resolution-and-loading-algorithm.
if (job.ts && path.startsWith(job.base) && path.slice(job.base.length).indexOf(sep + 'node_modules' + sep) === -1 && await job.isFile(path + '.ts')) return path + '.ts'; | ||
if (job.ts && path.startsWith(job.base) && path.slice(job.base.length).indexOf(sep + 'node_modules' + sep) === -1 && await job.isFile(path + '.tsx')) return path + '.tsx'; |
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.
Perhaps abstract this into an extensions
configuration rather, so it can be configured and not necessarily the default (sincethis will match ./file.ts
before file.js
).
Okay I'll go over this again soon. Side note: it seems like a shame to have all these separate implementations of the node module resolution algorithm. Is there really no package that's flexible enough to be used in all of these cases? I've used the |
fixes #266
Supports trailing characters after the
*
in wildcardexports
patterns, e.g.I copied the logic from Node's resolve implementation.