Parcel V3 - Native asset request #9808
Conversation
| @@ -150,9 +155,20 @@ impl Asset { | |||
| } | |||
| } | |||
|
|
|||
| impl Hash for Asset { | |||
There was a problem hiding this comment.
I removed this as we no longer need it to be hashable
| ) -> Result<RequestResult<AssetResult>, RunRequestError> { | ||
| request_context.report(ReporterEvent::BuildProgress(BuildProgressEvent::Building( | ||
| AssetBuildEvent { | ||
| // TODO: Should we try avoid a clone here? |
There was a problem hiding this comment.
Generally it'll be fine to clone.
If we find either copying memory or, more likely, memory allocation are a bottleneck, we can optimise easily for all read-only types using references or interning, but without having to spread lifetimes throughout.
There was a problem hiding this comment.
you'll want interning, it makes a big difference.
There was a problem hiding this comment.
Will address interning in a later PR
| TransformationInput::Asset(asset) => asset.file_path(), | ||
| } | ||
| } | ||
|
|
||
| pub fn read_source_code(&self, fs: FileSystemRef) -> anyhow::Result<Rc<SourceCode>> { | ||
| pub fn read_source_code(self, fs: FileSystemRef) -> anyhow::Result<Rc<SourceCode>> { |
There was a problem hiding this comment.
why does it consume self? you'd want to store Option<Rc<SourceCode>> in the initial asset to prevent consuming things here.
There was a problem hiding this comment.
This was a typo actually I think 😅
| } | ||
|
|
||
| pub fn validators(&self, _path: &Path) -> Result<Vec<Box<dyn ValidatorPlugin>>, anyhow::Error> { | ||
| todo!() | ||
| } | ||
| } | ||
|
|
||
| pub struct TransformerPipeline { | ||
| pub transformers: Vec<Box<dyn TransformerPlugin>>, |
There was a problem hiding this comment.
Still think this could just be a list of plugin names from the config that could be "loaded" on demand as they are run.
| ) -> Result<RequestResult<AssetResult>, RunRequestError> { | ||
| request_context.report(ReporterEvent::BuildProgress(BuildProgressEvent::Building( | ||
| AssetBuildEvent { | ||
| // TODO: Should we try avoid a clone here? |
There was a problem hiding this comment.
you'll want interning, it makes a big difference.
|
|
||
| let result = run_pipeline( | ||
| pipeline, | ||
| TransformationInput::InitialAsset(InitialAsset { |
There was a problem hiding this comment.
Can the input to a transformer always be an asset? Ideally transformers shouldn't know or care that they are first in a pipeline
There was a problem hiding this comment.
The idea here is to abstract this away via methods, but definitely happy to iterate on this design.
| let content_key = result.asset.id().to_string(); | ||
| self | ||
| .cache | ||
| .set_blob(&content_key, result.asset.source_code.bytes())?; |
There was a problem hiding this comment.
source_code is confusing since it's not source anymore it's compiled.
There was a problem hiding this comment.
Agreed, renamed to Code
| // TODO: move the following to RunRequestContext | ||
| pub cache: CacheRef, | ||
| pub file_system: FileSystemRef, | ||
| pub plugins: Arc<Plugins<'a>>, |
There was a problem hiding this comment.
I think this isn't quite ideal because it represents the entire config? So if you change the .parcelrc but only the packagers section for example, it'll invalidate all of the asset requests too. In my branch I only pass through a reference to the transformers section to avoid this.
There was a problem hiding this comment.
We were thinking that plugins would be tracked via invalidations rather than via the request id/hash. This is why I haven't added them to the hash
| self.pipeline.hash(state); | ||
| self.env.hash(state); | ||
| self.side_effects.hash(state); | ||
| } |
There was a problem hiding this comment.
Missing plugins? Should invalidate the request when the config changes.
There was a problem hiding this comment.
Agreed, we will address this later either with invalidations or by including the transformers in the hash.
This PR adds the initial logic for a native AssetRequest. Highly inspired by @devongovett's core-rs2 branch.
I haven't added any tests as yet and I'm a little unsure about whether they're worth it. Unlike the path request, this request needs access to the entire plugins struct which is currently painful to mock/simulate. Happy to be guided here if peeps have bright ideas to solve this.
There's still a bunch of TODO's left to resolve. We can address them now through the PR comments or come back to them later. I think the code will be easier to revisit once the transformers are in a working place.