Lookup alias and import maps using Pattern instead of String #8852
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Skipped Deployments
|
🟢 Turbopack Benchmark CI successful 🟢Thanks |
|
✅ This change can build |
|
mischnic
force-pushed
the
mischnic/pack-2801-pattern-in-alias-map
branch
from
dd8d510 to
cdf9cf7
Compare
mischnic
force-pushed
the
mischnic/pack-2801-pattern-in-alias-map
branch
from
cdf9cf7 to
5e13760
Compare
2 tasks
mischnic
force-pushed
the
mischnic/pack-2801-pattern-in-alias-map
branch
from
74e54a0 to
5c0005f
Compare
mischnic
force-pushed
the
mischnic/pack-2801-pattern-in-alias-map
branch
from
5c0005f to
4d7072a
Compare
mischnic
force-pushed
the
mischnic/pack-2801-pattern-in-alias-map
branch
from
4d7072a to
c4e171e
Compare
|
So now the output of const messages = await import(`@/messages/${locale}.json`);is const messages = await __turbopack_module_context__({
"./messages/de.json": {
id: ()=>"[project]/messages/de.json (json, async loader)",
module: ()=>__turbopack_require__("[project]/messages/de.json (json, async loader)")(__turbopack_import__)
},
"./messages/en.json": {
id: ()=>"[project]/messages/en.json (json, async loader)",
module: ()=>__turbopack_require__("[project]/messages/en.json (json, async loader)")(__turbopack_import__)
}
}).import(`@/messages/${locale}.json`)which fails because But apart from that, this PR is ready |
sokra
reviewed
Jul 30, 2024
| let common_prefixes = self.map.common_prefixes(request.as_bytes()); | ||
| let common_prefixes = self | ||
| .map | ||
| .common_prefixes(request.constant_prefix().as_bytes()); |
There was a problem hiding this comment.
This will lead to also returning non-matching results. e. g. Pattern could be "a" | "b" which has no constant_prefix so it would return "c" too.
You probably want:
- Do multiple lookups internally when you have Alternatives
- Filter the results by matching with the Pattern
There was a problem hiding this comment.
Looks like this doesn't happen, map.common_prefixes("") is just empty. That does align with example on https://docs.rs/patricia_tree/latest/patricia_tree/map/type.PatriciaMap.html#method.common_prefixes
But still, map.lookup(Pattern::Alternative(...)) currently never returns anything even if some/all alternatives match. And even then, just returning enum AliasMatch<R> {Exact(R), Replaced(R)} is wrong anyway because some might be exact and some might be replaced matches
sokra
reviewed
Jul 30, 2024
| let common_prefixes = self.map.common_prefixes(request.as_bytes()); | ||
| let common_prefixes = self | ||
| .map | ||
| .common_prefixes(request.constant_prefix().as_bytes()); |
sokra
reviewed
Jul 30, 2024
sokra
reviewed
Jul 30, 2024
sokra
reviewed
Jul 30, 2024
mischnic
added a commit
to vercel/next.js
that referenced
this pull request
Aug 16, 2024
Including vercel/turborepo#8852 Fixes #63372 Fixes PACK-2801 - [x] Change `RequestKey` handling: vercel/turborepo#8852 (comment) - [x] Fix lookup for alternatives: vercel/turborepo#8852 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The changes I had to make got out of hand quite quickly, because operating on
Patterns everywhere is more cumbersome than strings...Furthermore, some structs contained strings (but unrelated to requests being treated as strings, rather because import maps are specified as strings with wildcards), so now we needed a version with patterns.
This is almost working, namely the dynamic
@/*alias is already rewritten correctly now:Closes PACK-2801
Related to vercel/next.js#63372