perf(resolve): refactor tryFsResolve and tryResolveFile #12542
Conversation
|
|
|
|
||
| // Dependencies like es5-ext use `#` in their paths. We don't support `#` in user | ||
| // source code so we only need to perform the check for dependencies. | ||
| // We don't support `?` in node_modules paths, so we only need to check in this branch. |
There was a problem hiding this comment.
The function no longer used the splitFileAndPostfix util because we can avoid a redundant check if we know the result of the two indexOf.
There was a problem hiding this comment.
if the index is used later and it is important to split path from query and hash, foo#something?bla could trip it
There was a problem hiding this comment.
It was always the case that foo#something?bla would be split as foo#something + ?bla because a URL needs to have the ? before the #. Check how splitFileAndPostfix is working (that is being used in other places). If something is broken for node_modules path that has #, we are missing a test. Do you see a case that wouldn't work? (there definitely could be out of these two functions, but this PR shouldn't change that)
There was a problem hiding this comment.
If it works like the previous implementation it's fine i guess (at least not a regression). But from a url perspective, a hash containing a questionmark is allowed, right? so https://example.com/path#foo?bar would have no query and hash foo?bar.
So to safely extract the path, you'd have to use the lower first index of both.
patak-dev
added
p4-important
| if ( | ||
| options.mainFields[0] === 'sass' && | ||
| !options.extensions.includes(path.extname(entry)) | ||
| ) { | ||
| entry = '' | ||
| options.skipPackageJson = true |
There was a problem hiding this comment.
This was modifying the options object for every entry. I think there was a bug here we never catched. I refactored skipPackageJson to pass it directly to tryFsResolve now.
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
|
✅ Astro is back green! 🎉 |
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
|
|
||
| // Dependencies like es5-ext use `#` in their paths. We don't support `#` in user | ||
| // source code so we only need to perform the check for dependencies. | ||
| const tryUnsplitted = fsPath.includes('#') && fsPath.includes('node_modules') |
There was a problem hiding this comment.
If there was a # before the query, before we were checking for tryUnsplitted branches with the full path including the query and these were all redundant checks.
Description
tryFsResolvewas callingtryResolveFile, that recursively would calltryFsResolveif the file was a directory. The code was quite complex to follow, and optimizations were added to short-circuit some redundant checks.The function had to deal with:
#in anode_modulespath/index->_indexfor .sass)This PR deals with the
#innode_modulespath and postfix upfront intryFsResolve, creating a newtryCleanFsResolvethat doesn't need to deal with the first two requirements.This means that from now on if there is a
#innode_modules, we will first check the whole resolution for the completefsPathfirst. Before, these checks were interleaved. I think this is the proper order, and it shouldn't affect much because only a few deps have#in their paths.The PR also removes
tryFsResolve, flattening the checks directly intryCleanFsResolve. The new function is more verbose but we no longer have recursion intotryFsResolve. All the checks and the order are now clear by looking at this function (it is terrible how many things we need to check).Because all the checks are in the same function, it is easier to reuse
statSynccalls and to group all the checks that needpath.dirname(file)to be a directory and bail out from all of them if not.What is the purpose of this pull request?