refactor asset identification #3999
Merged
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 Ignored Deployments
|
alexkirsz
reviewed
Feb 28, 2023
| for param in ident.params.iter() { | ||
| match *param { | ||
| AssetParam::Query(query) => { | ||
| 0_u8.deterministic_hash(&mut hasher); |
There was a problem hiding this comment.
We need a DeterministicHash derive.
There was a problem hiding this comment.
That won't work here well since it is async. We would need a AsyncDeterministicHash where deterministic_hash is an async method. That's a lot work...
Benchmark for e9524fdClick to view benchmark
|
sokra
force-pushed
the
sokra/refactor-asset-ident
branch
from
02ac202
to
86a36fb
Compare
Benchmark for b370524
Click to view full benchmark
|
|
Benchmark for 471bfb8
Click to view full benchmark
|
add ChunkItem::ident() instead of ChunkItem::to_string() base chunk_path on AssetIdent instead of path only
make AssetIdent having an identity
sokra
force-pushed
the
sokra/refactor-asset-ident
branch
from
a4afc6a
to
aff7e79
Compare
sokra
force-pushed
the
sokra/refactor-asset-ident
branch
from
aff7e79
to
51b3b69
Compare
Benchmark for a7827ce
Click to view full benchmark
|
alexkirsz
approved these changes
Mar 1, 2023
|
|
||
| "[project]/crates/turbopack-tests/tests/snapshot/basic/async_chunk/input/import.js/manifest-loader.js": (({ r: __turbopack_require__, x: __turbopack_external_require__, i: __turbopack_import__, s: __turbopack_esm__, v: __turbopack_export_value__, c: __turbopack_cache__, l: __turbopack_load__, j: __turbopack_cjs__, p: process, g: global, __dirname }) => (() => { | ||
| "[project]/crates/turbopack-tests/tests/snapshot/basic/async_chunk/input/import.js (ecmascript, manifest chunk, loader)": (({ r: __turbopack_require__, x: __turbopack_external_require__, i: __turbopack_import__, s: __turbopack_esm__, v: __turbopack_export_value__, c: __turbopack_cache__, l: __turbopack_load__, j: __turbopack_cjs__, p: process, g: global, __dirname }) => (() => { |
sokra
added a commit
that referenced
this pull request
Mar 2, 2023
### Description * get rid of attached filesystem for our embedded modules * get rid of import "." in favor of inner assets depends on #3999 ### Testing Instructions existing tests <!-- When the below is checked (default) our PR bot will automatically assign labels to your PR based on the content to help the team organize and review it faster. --> [X] Auto label
ijjk
added a commit
to vercel/next.js
that referenced
this pull request
Mar 2, 2023
# New Features * vercel/turbo#4011 # Performance Improvements * vercel/turbo#3955 * vercel/turbo#4018 # Bug Fixes * vercel/turbo#4037 * vercel/turbo#4028 # Other * vercel/turbo#3974 * vercel/turbo#4015 * vercel/turbo#3999 * vercel/turbo#4026 * vercel/turbo#4053 * vercel/turbo#3891 Co-authored-by: JJ Kasper <jj@jjsweb.site>
jridgewell
pushed a commit
to vercel/next.js
that referenced
this pull request
Mar 10, 2023
* add Asset::ident() as unique identifier of an Asset * add ChunkItem::ident() instead of ChunkItem::to_string() (ValueToString) * base `chunk_path` on AssetIdent instead of path only Motivation: We want to get rid of the `import "."` in favor of inner assets. When doing this we no longer need to place virtual assets below the actual file path and they can stay in their original path. But placing virtual assets below the actual asset also made the `Asset::path` unique, which would no longer be the case after using inner assets. Some parts of the code base relied on `Asset::path` being unique (e. g. module ids and chunk paths). But actually we never guaranteed that to be unique. After this PR `Asset::ident` is intended to be unique and allow to carry more information than only the path: * Query string (`module?query`) * Fragment (`module#fragment`) * Asset (additional wrapped assets by key value pairs) * Modifiers (additional transformations applied on the module, e. g. `chunks`, `client chunks`) * In future: Part (select a subpart of the module, e. g. only export abc, or the module evaluation, or some internal part)
jridgewell
pushed a commit
to vercel/next.js
that referenced
this pull request
Mar 10, 2023
### Description * get rid of attached filesystem for our embedded modules * get rid of import "." in favor of inner assets depends on vercel/turbo#3999 ### Testing Instructions existing tests <!-- When the below is checked (default) our PR bot will automatically assign labels to your PR based on the content to help the team organize and review it faster. --> [X] Auto label
sokra
added a commit
to vercel/next.js
that referenced
this pull request
Mar 13, 2023
* add Asset::ident() as unique identifier of an Asset * add ChunkItem::ident() instead of ChunkItem::to_string() (ValueToString) * base `chunk_path` on AssetIdent instead of path only Motivation: We want to get rid of the `import "."` in favor of inner assets. When doing this we no longer need to place virtual assets below the actual file path and they can stay in their original path. But placing virtual assets below the actual asset also made the `Asset::path` unique, which would no longer be the case after using inner assets. Some parts of the code base relied on `Asset::path` being unique (e. g. module ids and chunk paths). But actually we never guaranteed that to be unique. After this PR `Asset::ident` is intended to be unique and allow to carry more information than only the path: * Query string (`module?query`) * Fragment (`module#fragment`) * Asset (additional wrapped assets by key value pairs) * Modifiers (additional transformations applied on the module, e. g. `chunks`, `client chunks`) * In future: Part (select a subpart of the module, e. g. only export abc, or the module evaluation, or some internal part)
sokra
added a commit
to vercel/next.js
that referenced
this pull request
Mar 13, 2023
### Description * get rid of attached filesystem for our embedded modules * get rid of import "." in favor of inner assets depends on vercel/turbo#3999 ### Testing Instructions existing tests <!-- When the below is checked (default) our PR bot will automatically assign labels to your PR based on the content to help the team organize and review it faster. --> [X] Auto label
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.
chunk_pathon AssetIdent instead of path onlyMotivation:
We want to get rid of the
import "."in favor of inner assets. When doing this we no longer need to place virtual assets below the actual file path and they can stay in their original path. But placing virtual assets below the actual asset also made theAsset::pathunique, which would no longer be the case after using inner assets. Some parts of the code base relied onAsset::pathbeing unique (e. g. module ids and chunk paths). But actually we never guaranteed that to be unique.After this PR
Asset::identis intended to be unique and allow to carry more information than only the path:module?query)module#fragment)chunks,client chunks)