Parcel 2: Asset methods #2335
Parcel 2: Asset methods #2335
Conversation
| } | ||
|
|
||
| if (result.ast && context.generate) { |
There was a problem hiding this comment.
Wow, this change really makes the TransformerRunner easier to follow. Good work!
| } | ||
|
|
||
| toJSON(): AssetOptions { | ||
| // Exclude `code` and `ast` from cache |
There was a problem hiding this comment.
Would it make sense to also set code to null when the asset is finalized in TransformerRunner? I noticed these fields are getting removed when the asset is stringified going into the cache, but that's not what is passed back on a cache miss run, so some "code" is being stored in memory in the graph.
There was a problem hiding this comment.
Probably. That's true, although when run in the worker farm the assets are converted to JSON to pass over the IPC back to the main process.
There was a problem hiding this comment.
Yeah I guess it wouldn't be every cache miss, just the cache misses before the workers warm up. Still probably better to guarantee it's removed once finalized though.
| let cacheEntry = await this.runPipeline( | ||
| nextInput, | ||
| pipeline.slice(1), | ||
| null, | ||
| getNextContext(context, result) | ||
| generate | ||
| ); | ||
|
|
||
| assets = assets.concat(cacheEntry.assets); | ||
| connectedFiles = connectedFiles.concat(cacheEntry.connectedFiles); |
There was a problem hiding this comment.
I got a little confused here because I thought that cacheEntry was only passed in to the first call of runPipeline. I guess it has part of the shape of a CacheEntry, but it might help to not give it the same name as a function parameter. Maybe nextPipelineResult?
There was a problem hiding this comment.
👍 originally it was a full cache entry and then I changed it
| @@ -85,8 +85,7 @@ export type SourceLocation = { | |||
| end: {line: number, column: number} | |||
| }; | |||
|
|
|||
| export type Dependency = { | |||
| sourcePath: FilePath, | |||
| export type DependencyOptions = { | |||
There was a problem hiding this comment.
Kinda not feeling this name. What about DependencyDescription?
| @@ -98,6 +97,16 @@ export type Dependency = { | |||
| meta?: JSONObject | |||
| }; | |||
|
|
|||
| export type Dependency = { | |||
There was a problem hiding this comment.
Wondering if this should be DependencyRequest? Kinda makes sense since it is a combination of a dependency "description" (or whatever we decide to name it) and the site where the dependency was requested from.
There was a problem hiding this comment.
The idea was that DependencyOptions was the options used to construct a Dependency. Maybe we could just unify them into one type. I just didn't like all the flow warnings created by some fields being optional (like id).
Alternatively, we could have Dependency be what the user specifies, and ResolverRequest be the one with the id on it. But then we would have to rename the dependencies field on an asset?
| @@ -136,28 +111,27 @@ class TransformerRunner { | |||
| } | |||
|
|
|||
| // If the generated asset has the same type as the input... | |||
| if (result.type === context.type) { | |||
| // TODO: this is incorrect since multiple file types could map to the same pipeline. need to compare the pipelines. | |||
There was a problem hiding this comment.
Could you give an example of when this might happen?
There was a problem hiding this comment.
i.e. in the default config, we have *.{js,mjs,es6} mapping to a single transformer pipeline. If someone had an .mjs file as input, but the output was always js from the transformer, then we'd get into an infinite loop here. So we should be comparing the pipelines, not the extensions.
There was a problem hiding this comment.
I guess I was thinking those would all still by type "js". Is type ever different than the extension?
There was a problem hiding this comment.
Well we initialize type to the file extension at the start of the pipeline:
https://github.com/parcel-bundler/parcel/pull/2335/files#diff-e00a1c679d921332a0033bb784c5d4b7R54 This means that the input type might be mjs but the output of the transformer would be js. Currently the code would recurse and run the JS transformer again with an input type of js rather than stopping. So, we need to compare the pipeline rather than the type.
| ); | ||
|
|
||
| assets = assets.concat(cacheEntry.assets); | ||
| connectedFiles = connectedFiles.concat(cacheEntry.connectedFiles); | ||
| } | ||
| } else { | ||
| // Jump to a different pipeline for the generated asset. | ||
| let nextInput = transformerResultToInput(input, result); | ||
| let nextInput = input.createChildAsset(result); |
There was a problem hiding this comment.
Looks like this is already being done outside the if/else
|
Going to merge so I can use this in other work. |
This is an experiment with creating a mutable Asset class with some helper methods for transformers to use rather than only returning plain objects. The methods include things like
getConfigandgetPackagehelpers, along withaddDependency,addConnectedFile, andcreateChildAsset` mutation/factory methods. None of these are required - plain objects also work fine, and are converted on the fly by the transformer runner. I think it simplifies transformers quite a bit, and also simplifies the transformer runner.