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

Resolver improvements #6696

Merged
merged 12 commits into from
Aug 16, 2021
Merged

Resolver improvements #6696

merged 12 commits into from
Aug 16, 2021

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Aug 9, 2021

While trying to document the resolver, I came across some inconsistencies and opportunities for improvement.

  • File extensions are always required for URL dependencies. For ESM and CJS specifiers, only the following extensions may be omitted: ['ts', 'tsx', 'js', 'jsx', 'json'].
  • Added some additional reserved pipeline names, which conflict with common URL schemes. Some of these may be handled by the default resolver in the future, or by custom resolver plugins.
  • Don't support resolving directories from URL dependencies (either package.json or index files)
  • Allow resolver plugins to handle more aspects of URL parsing. Previously, core would return early for URL dependencies with schemes that didn't match a pipeline, or throw for ESM. Core also removed query parameters before passing the specifier to resolver plugins. Now it does none of this, and lets plugins decide what to do. Plugins can return query params that they parsed and didn't handle. This also fixes parsing ESM specifiers as URLs, so that things like percent encoding are supported properly.
  • CommonJS no longer supports query parameters. This matches node's behavior, where the ? character is allowed in filenames (just not on Windows...).
  • Adds support for an npm: scheme, which allows URL dependencies to resolve to node_modules packages rather than as a relative path. Useful for e.g. CSS and web workers. Fixes Support Web Workers coming from node_modules #670. Closes Parcel 2: Specific resolvers per file type #3492.
  • Fixes the CSS transformer to pass external URLs through resolvers rather than leaving them inline. Hoists all @import declarations to the top of the packaged bundle. Fixes CSS @imports are not the first rule in the css output file #5484.
  • Renames the filePath option of resolver plugins to specifier since it may include things like query params and URL schemes. Note that this is slightly different from dependency.specifier in cases where a known named pipeline is used. In this case, the specifier passed to resolvers already has the pipeline stripped off.

@height
Copy link

height bot commented Aug 9, 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.

@parcel-benchmark
Copy link

parcel-benchmark commented Aug 9, 2021

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.58s +12.00ms
Cached 263.00ms +9.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 461.00ms -38.00ms 🚀
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 462.00ms -38.00ms 🚀
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 462.00ms -37.00ms 🚀

Cached Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 471.00ms -27.00ms 🚀
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 471.00ms -27.00ms 🚀
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 472.00ms -27.00ms 🚀

React HackerNews ✅

Timings

Description Time Difference
Cold 8.06s -105.00ms
Cached 414.00ms -2.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 57.06s +1.05s
Cached 1.51s -4.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/index.04eaaaa3.js 1.77mb +4.00b ⚠️ 16.53s +322.00ms
dist/index.d2f714b9.js 694.59kb +15.00b ⚠️ 43.83s +1.10s
dist/Modal.6d102ab0.js 45.30kb +15.00b ⚠️ 43.34s +1.06s

Cached Bundles

Bundle Size Difference Time Difference
dist/index.d4ffc1c1.js 1.77mb +6.00b ⚠️ 16.05s -123.00ms
dist/EmojiPickerComponent.513df8b1.js 146.66kb +0.00b 17.53s -8.80s 🚀
dist/esm.d037b5a8.js 33.13kb +0.00b 17.53s -8.80s 🚀
dist/DatePicker.69b3c4ae.js 22.96kb +0.00b 17.53s -8.80s 🚀
dist/js.0f0bb621.js 17.24kb +0.00b 31.95s -10.14s 🚀
dist/png-chunks-extract.94b5b9fc.js 3.56kb +0.00b 31.95s -10.14s 🚀
dist/feedback.e18b45f0.js 1.75kb +0.00b 17.53s -8.80s 🚀
dist/workerHasher.567cfc57.js 1.62kb +0.00b 17.53s -8.80s 🚀
dist/heading6.b3b946d1.js 1.49kb +0.00b 17.53s -8.80s 🚀
dist/heading5.e617db66.js 1.37kb +0.00b 17.53s -8.80s 🚀
dist/expand.1f17de7f.js 1.27kb +0.00b 17.53s -8.80s 🚀

Three.js ✅

Timings

Description Time Difference
Cold 6.03s +30.00ms
Cached 342.00ms -12.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@@ -8,7 +8,7 @@ import NodeResolver from '@parcel/node-resolver-core';
const WEBPACK_IMPORT_REGEX = /\S+-loader\S*!\S+/g;

export default (new Resolver({
resolve({dependency, options, filePath}) {
resolve({dependency, options, specifier}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 this name makes so much more sense

@@ -1452,6 +1452,7 @@ export type ResolveResult = {|
/** An absolute path to the file. */
+filePath?: FilePath,
+pipeline?: ?string,
+query?: QueryParameters,
Copy link
Member

Choose a reason for hiding this comment

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

Can you document this? (That these are the query params the resolver didn't handle and should be forwarded to transformers)

if (!validPipelines.has(pipeline)) {
// This may be a url protocol or scheme rather than a pipeline, such as
// `url('http://example.com/foo.png')`. Pass it to resolvers to handle.
// return {assetGroup: null};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// return {assetGroup: null};

@devongovett devongovett merged commit aa21e89 into v2 Aug 16, 2021
@devongovett devongovett deleted the resolver-fixes branch August 16, 2021 01:52
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