-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
perf(resolve): support # in path only for dependencies #12469
Conversation
Run & review this pull request in StackBlitz Codeflow. |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
Ecosystem CI looks fine, same fails as in main (SvelteKit latest main is failing rigth now) |
let res: string | undefined | ||
|
||
// if there is a postfix, try resolving it as a complete path first (#4703) | ||
if ( |
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 think we can add some comments before each if
branch. The comments tell what fsPath
will hit these if
conditions. It's helpful for understanding these codes quickly and refactoring them later. And I guess there are still rooms to dedupe resolve logic.
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 renamed the variable instead of adding a new comment because I think the main comment for tryUnsplitted
is enough now.
Description
@sun0day has been investigating
tryFsResolve
double checks to support paths with#
. See:We added the checks to resolve the file path without removing the postfix because of dependencies like es5-ext here:
But the use of
?
and#
never worked in user code and this is something we can control. Requiring users to avoid?
and#
in their paths is a fair price to pay to avoid double checks.This PR formalizes that we only support
#
in dependencies paths, and removes the double checks in other cases. Note: I only included support for#
here, and not?
so the double checks won't be triggered for every dependency.What is the purpose of this pull request?