Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for worklets and fix pipeline bugs #6495

Merged
merged 11 commits into from
Jun 24, 2021
Merged

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Jun 21, 2021

Fixes #1093, Fixes #2083, Fixes #4174, Closes #5837, Closes #6510

This adds support for worklets including the CSS.paintWorklet.addModule syntax, as well as for audio worklets via the worklet: pipeline. The pipeline is needed in that case because the API is not statically analyzable. Worklets are a new environment context. While they are similar to web workers, they have some unique constraints: only ESM is supported (not scripts), but dynamic import() is not supported (as with other networking APIs like fetch). Parcel will give you a build time error for this.

This also fixes several other issues that are somewhat related.

  • Using the url: pipeline with a JavaScript file was broken because the url: pipeline did not run other transformers before the raw transformer, meaning the JS wasn't transformed.
  • Using new URL with a JS file was also broken because the generated JS would never run its entry asset. This was because it was not marked as isolated, and so thought the parent bundle would parcelRequire it like with normal async imports. Also, it assumed it had access to the modules in parent bundles, but this wasn't necessarily true. We now mark URL dependencies in JS files as isolated. This required exposing bundleBehavior to dependencies like we do for assets.
  • Referencing JS files with bundle-text: or other pipelines that trigger inline bundles was broken when HMR was enabled. This was because we took the first entry asset rather than the main entry when checking asset.meta.inlineType.
  • Isolated bundles now affect the parallel request limit properly. Previously all isolated bundles were connected to the root in the graph rather than where they came from. This made some reachability checks easier, but meant that it wasn't possible to have an isolated bundle at a parallel priority, only at a lazy priority.
  • Fixes usage of dynamic import with named pipelines. Previously, we ignored these as they appeared like URLs. But importing URLs was also broken and we'll need to find a different solution for that anyway.
  • Adds support for dynamically importing a dependency that turns out to be inline (e.g. import('bundle-text:./foo'). Previously this was just broken, but now it returns a Promise.resolve(contents).

@height
Copy link

height bot commented Jun 21, 2021

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

asset.bundleBehavior === 'inline'
? false
: dependency.isEntry || dependency.needsStableName,
isInline: asset.bundleBehavior === 'inline',
bundleBehavior: dependency.bundleBehavior ?? asset.bundleBehavior,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one should win? 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say keep it bundleBehavior, it's easier to search since it's all the same feature

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @devongovett is asking if the dependency's behavior or asset's behavior should be preferred

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... I must be going crazy, I swore I commented on the comment asking #6495 (comment)

Maybe the 👁️ doctor messed up this morning hah.

In this case, what's more regular case to test against an asset being inline or isolate or a dependency being inline or isolated? And what's the expectation if the dep and asset's bundle behavior were separate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think the dep should win because the importing code might have an expectation about what the asset exposes.

* - isolated: The resolved asset will be isolated from its parents in a separate bundle.
* Shared assets will be duplicated.
*/
+bundleBehavior?: BundleBehavior,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API wise I'm not too sure about this. I considered combining bundleBehavior and priority, but I think it's possible to have an isolated bundle that is either parallel or lazy (but probably not sync), and it's possible to have an inline sync or async (though parallel vs lazy doesn't really matter). 🤔

@@ -1106,8 +1130,7 @@ export interface Bundle {
+env: Environment;
/** Whether this is an entry (e.g. should not be hashed). */
+isEntry: ?boolean;
/** Whether this bundle should be inlined into the parent bundle(s), */
+isInline: ?boolean;
+bundleBehavior: ?BundleBehavior;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is bundle.bundleBehavior too redundant? bundle.behavior??

loc: convertLoc(dep.loc),
});
} else if (dep.kind === 'File') {
asset.invalidateOnFileChange(dep.specifier);
} else {
if (dep.kind === 'DynamicImport' && isURL(dep.specifier)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was broken already because we already replaced the import() with a require() in rust. The isUrl check was interfering with usage of pipelines in dynamic import.

@parcel-benchmark
Copy link

parcel-benchmark commented Jun 21, 2021

Benchmark Results

Kitchen Sink 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

React HackerNews ✅

Timings

Description Time Difference
Cold 9.94s +38.00ms
Cached 459.00ms +20.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js ✅

Timings

Description Time Difference
Cold 7.11s +103.00ms
Cached 398.00ms -1.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

…endencies

# Conflicts:
#	packages/bundlers/default/src/DefaultBundler.js
#	packages/core/core/src/dumpGraphToGraphViz.js
#	packages/core/core/src/public/MutableBundleGraph.js
#	packages/core/core/src/types.js
#	packages/core/core/test/PublicBundle.test.js
#	packages/core/types/index.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants