Packager request #6379
Merged
Packager request #6379
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
|
This pull request has been linked to and will mark 2 tasks as "Done" when merged:
|
devongovett
commented
May 31, 2021
| @@ -1458,7 +1450,9 @@ export default class BundleGraph { | |||
|
|
|||
| getHash(bundle: Bundle): string { | |||
| let hash = new Hash(); | |||
| hash.writeString(bundle.id + this.getContentHash(bundle)); | |||
| hash.writeString( | |||
| bundle.id + bundle.target.publicUrl + this.getContentHash(bundle), | |||
There was a problem hiding this comment.
Target options aren't in the environment hash and aren't tracked in the graph. This is kinda assuming that packagers will use the publicUrl in some way, but currently this is only true of the HTML packager. Perhaps we can make this more granular in the future somehow...
devongovett
commented
May 31, 2021
|
|
||
| await this.#packagerRunner.writeBundles(bundleGraph); | ||
| assertSignalNotAborted(signal); | ||
| } = await this.#requestTracker.runRequest(request, {force: true}); |
There was a problem hiding this comment.
Currently we force the parcel build request to run every time, even when nothing changed. This doesn't cause the sub-requests to re-run – they will return cached results. But if we want to emit a build success event with the bundle graph for builds where nothing actually changed, we need to do this to get the results.
There was a problem hiding this comment.
Could the the parcel build request do api.storeResult instead of only returning it? Not sure if that's more overhead than just rerunning
devongovett
commented
May 31, 2021
| } | ||
|
|
||
| _getOptimizerNodes( | ||
| filePath: FilePath, | ||
| pipeline: ?string, | ||
| ): PureParcelConfigPipeline { | ||
| // If a pipeline is specified, but it doesn't exist in the optimizers config, ignore it. | ||
| // Pipelines for bundles come from their entry assets, so the pipeline likely exists in transformers. |
devongovett
commented
May 31, 2021
| @@ -282,7 +283,26 @@ export class PackagedBundle extends NamedBundle implements IPackagedBundle { | |||
| return packagedBundle; | |||
| } | |||
|
|
|||
| static getWithInfo( | |||
There was a problem hiding this comment.
I couldn't get Flow to let me add an additional argument to the existing get function...
devongovett
commented
May 31, 2021
| // Watch the bundle and source map for deletion. | ||
| // Also watch the dist dir because invalidateOnFileDelete does not currently | ||
| // invalidate when a parent directory is deleted. | ||
| // TODO: do we want to also watch for file edits? |
There was a problem hiding this comment.
I thought it might be weird if a user wanted to manually edit a file in the dist directory for some reason (e.g. debugging), and Parcel immediately overwrote it
devongovett
commented
May 31, 2021
| /** | ||
| * Packages, optimizes, and writes all bundles to the dist directory. | ||
| */ | ||
| export default function createWriteBundlesRequest( |
There was a problem hiding this comment.
Is there a better name for this request? It's very similar to the singular WriteBundleRequest. This one does the entire process of packaging, optimizing, and writing every bundle. Open to name suggestions here.
devongovett
commented
May 31, 2021
| get filePath(): string { | ||
| return nullthrows(this.#bundle.filePath); | ||
| return nullthrows(this.#bundleInfo).filePath; |
There was a problem hiding this comment.
Previously, we mutated the bundle after packaging to set the file path we actually wrote to. This has a number of problems with caching, so I moved this to a separate bundle info object. It's transparent to the end user, but internally, this is not stored on the actual bundle node.
|
One other question is how granular we want to get. Right now, packaging and optimizing a bundle is a single request, and they are cached together, but we could make the separate requests if we wanted. |
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached Bundles
React HackerNews ✅
Timings
Cold Bundles
Cached Bundles
AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached Bundles
|
# Conflicts: # Cargo.lock # Cargo.toml # packages/core/core/src/BundleGraph.js # packages/core/core/src/PackagerRunner.js # packages/core/core/src/RequestTracker.js # packages/core/core/src/Transformation.js # packages/core/core/src/requests/ConfigRequest.js # packages/packagers/js/src/index.js # packages/transformers/postcss/src/PostCSSTransformer.js # packages/utils/hash/index.js # packages/utils/hash/index.js.flow # packages/utils/hash/package.json
This was referenced Jun 6, 2021
DeMoorJasper
approved these changes
Jun 7, 2021
mischnic
reviewed
Jun 9, 2021
Comment on lines
68
to
69
| // TODO: should we be mutating this here? | ||
| requestedAssetIds.clear(); |
There was a problem hiding this comment.
Maybe leave this part in Parcel.js and do it after the build request finished?
mischnic
reviewed
Jun 9, 2021
| optionsRef, | ||
| }); | ||
|
|
||
| // $FlowFixMe Added in Flow 0.121.0 upgrade in #4381 |
There was a problem hiding this comment.
Suggested change
| // $FlowFixMe Added in Flow 0.121.0 upgrade in #4381 |
wbinnssmith
approved these changes
Jun 9, 2021
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.
Depends on #6373. Closes T-764 T-765. Fixes #5755
This refactors packaging and optimizing into requests like everything else in Parcel, and adds invalidations to the request graph for everything that might cause us to need to repackage/optimize. This includes dev dependencies, config, options, etc.
In addition to packaging and optimizing, the process of writing each bundle to the dist directory is also a request now, which means we avoid the work of writing bundles to dist that haven't changed. This also tracks if the dist file was deleted manually by the user and rewrites it if needed.
Finally, the entire Parcel build process is now a request as well. Rather than the
Parcelclass calling each step, there is a singleParcelBuildRequestthat treats each of these steps as sub-requests. This ensures that old sub-requests get deleted from the request graph properly instead of leaking orphaned nodes.