Resolver improvements (#6696)

packages/core/core/src/requests/ParcelConfigRequest.js

@@ -264,6 +264,16 @@ function processPipeline(
   }
 }
 
+const RESERVED_PIPELINES = new Set([
+  'node:',
+  'npm:',
+  'http:',
+  'https:',
+  'data:',
+  'tel:',
+  'mailto:',
+]);
+
 async function processMap(
   // $FlowFixMe
   map: ?ConfigMap<any, any>,
@@ -277,12 +287,12 @@ async function processMap(
   // $FlowFixMe
   let res: ConfigMap<any, any> = {};
   for (let k in map) {
-    if (k.startsWith('node:')) {
+    let i = k.indexOf(':');
+    if (i > 0 && RESERVED_PIPELINES.has(k.slice(0, i + 1))) {
       let code = await options.inputFS.readFile(filePath, 'utf8');
       throw new ThrowableDiagnostic({
         diagnostic: {
-          message:
-            'Named pipeline `node:` is reserved for builtin Node.js libraries',
+          message: `Named pipeline '${k.slice(0, i + 1)}' is reserved.`,
           origin: '@parcel/core',
           codeFrames: [
             {

packages/core/core/src/requests/PathRequest.js

@@ -1,11 +1,6 @@
 // @flow strict-local
 import type {Diagnostic} from '@parcel/diagnostic';
-import type {
-  Async,
-  FileCreateInvalidation,
-  FilePath,
-  QueryParameters,
-} from '@parcel/types';
+import type {Async, FileCreateInvalidation, FilePath} from '@parcel/types';
 import type {StaticRunOpts} from '../RequestTracker';
 import type {AssetGroup, Dependency, ParcelOptions} from '../types';
 import type {ConfigAndCachePath} from './ParcelConfigRequest';
@@ -14,9 +9,7 @@ import ThrowableDiagnostic, {errorToDiagnostic, md} from '@parcel/diagnostic';
 import {PluginLogger} from '@parcel/logger';
 import nullthrows from 'nullthrows';
 import path from 'path';
-import URL from 'url';
 import {normalizePath} from '@parcel/utils';
-import querystring from 'querystring';
 import {report} from '../ReporterRunner';
 import PublicDependency from '../public/Dependency';
 import PluginOptions from '../public/PluginOptions';
@@ -50,7 +43,6 @@ type RunOpts = {|
 |};
 
 const type = 'path_request';
-const QUERY_PARAMS_REGEX = /^([^\t\r\n\v\f?]*)(\?.*)?/;
 const PIPELINE_REGEX = /^([a-z0-9-]+?):(.*)$/i;
 
 export default function createPathRequest(
@@ -166,8 +158,7 @@ export class ResolverRunner {
     let resolvers = await this.config.getResolvers();
 
     let pipeline;
-    let filePath;
-    let query: ?QueryParameters;
+    let specifier;
     let validPipelines = new Set(this.config.getNamedPipelines());
     let match = dependency.specifier.match(PIPELINE_REGEX);
     if (
@@ -176,82 +167,28 @@ export class ResolverRunner {
       // and include e.g. `C:\` on Windows, conflicting with pipelines.
       !path.isAbsolute(dependency.specifier)
     ) {
-      if (dependency.specifier.startsWith('node:')) {
-        filePath = dependency.specifier;
-      } else {
-        [, pipeline, filePath] = match;
-        if (!validPipelines.has(pipeline)) {
-          if (dep.specifierType === 'url') {
-            // This may be a url protocol or scheme rather than a pipeline, such as
-            // `url('http://example.com/foo.png')`
-            return {assetGroup: null};
-          } else {
-            return {
-              assetGroup: null,
-              diagnostics: [
-                await this.getDiagnostic(
-                  dependency,
-                  md`Unknown pipeline: ${pipeline}.`,
-                ),
-              ],
-            };
-          }
-        }
-      }
-    } else {
-      if (dep.specifierType === 'url') {
-        if (dependency.specifier.startsWith('//')) {
-          // A protocol-relative URL, e.g `url('//example.com/foo.png')`
-          return {assetGroup: null};
-        }
-        if (dependency.specifier.startsWith('#')) {
-          // An ID-only URL, e.g. `url(#clip-path)` for CSS rules
-          return {assetGroup: null};
-        }
-      }
-      filePath = dependency.specifier;
-    }
-
-    let queryPart = null;
-    if (dep.specifierType === 'url') {
-      let parsed = URL.parse(filePath);
-      if (typeof parsed.pathname !== 'string') {
-        return {
-          assetGroup: null,
-          diagnostics: [
-            await this.getDiagnostic(
-              dependency,
-              md`Received URL without a pathname ${filePath}.`,
-            ),
-          ],
-        };
-      }
-      filePath = decodeURIComponent(parsed.pathname);
-      if (parsed.query != null) {
-        queryPart = parsed.query;
+      [, pipeline, specifier] = match;
+      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.
+        specifier = dependency.specifier;
+        pipeline = null;
       }
     } else {
-      let matchesQuerystring = filePath.match(QUERY_PARAMS_REGEX);
-      if (matchesQuerystring && matchesQuerystring[2] != null) {
-        filePath = matchesQuerystring[1];
-        queryPart = matchesQuerystring[2].substr(1);
-      }
-    }
-    if (queryPart != null) {
-      query = querystring.parse(queryPart);
+      specifier = dependency.specifier;
     }
 
     // Entrypoints, convert ProjectPath in module specifier to absolute path
     if (dep.resolveFrom == null) {
-      filePath = path.join(this.options.projectRoot, filePath);
+      specifier = path.join(this.options.projectRoot, specifier);
     }
     let diagnostics: Array<Diagnostic> = [];
     let invalidateOnFileCreate = [];
     let invalidateOnFileChange = [];
     for (let resolver of resolvers) {
       try {
         let result = await resolver.plugin.resolve({
-          filePath,
+          specifier,
           pipeline,
           dependency: dep,
           options: this.pluginOptions,
@@ -302,7 +239,7 @@ export class ResolverRunner {
                   this.options.projectRoot,
                   resultFilePath,
                 ),
-                query,
+                query: result.query,
                 sideEffects: result.sideEffects,
                 code: result.code,
                 env: dependency.env,
@@ -322,7 +259,7 @@ export class ResolverRunner {
               new ThrowableDiagnostic({diagnostic: result.diagnostics}),
               {
                 origin: resolver.name,
-                filePath,
+                filePath: specifier,
               },
             );
             diagnostics.push(...errorDiagnostic);
@@ -332,7 +269,7 @@ export class ResolverRunner {
         // Add error to error map, we'll append these to the standard error if we can't resolve the asset
         let errorDiagnostic = errorToDiagnostic(e, {
           origin: resolver.name,
-          filePath,
+          filePath: specifier,
         });
         if (Array.isArray(errorDiagnostic)) {
           diagnostics.push(...errorDiagnostic);

packages/core/core/test/ParcelConfig.test.js

@@ -289,8 +289,7 @@ describe('ParcelConfig', () => {
           name: 'Error',
           diagnostics: [
             {
-              message:
-                'Named pipeline `node:` is reserved for builtin Node.js libraries',
+              message: "Named pipeline 'node:' is reserved.",
               origin: '@parcel/core',
               codeFrames: [
                 {

packages/core/integration-tests/test/css.js

@@ -381,4 +381,41 @@ describe('css', () => {
       },
     );
   });
+
+  it('should support importing CSS from node_modules with the npm: scheme', async () => {
+    let b = await bundle(
+      path.join(__dirname, '/integration/css-node-modules/index.css'),
+    );
+
+    assertBundles(b, [
+      {
+        name: 'index.css',
+        assets: ['index.css', 'foo.css'],
+      },
+    ]);
+  });
+
+  it('should support external CSS imports', async () => {
+    let b = await bundle(
+      path.join(__dirname, '/integration/css-external/a.css'),
+    );
+
+    assertBundles(b, [
+      {
+        name: 'a.css',
+        assets: ['a.css', 'b.css'],
+      },
+    ]);
+
+    let res = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
+    assert(
+      res.startsWith(`@import "http://example.com/external.css";
+.b {
+  color: red;
+}
+.a {
+  color: blue;
+}`),
+    );
+  });
 });

packages/core/integration-tests/test/integration/css-external/a.css

@@ -0,0 +1,6 @@
+@import 'b.css';
+@import 'http://example.com/external.css';
+
+.a {
+  color: blue;
+}

packages/core/integration-tests/test/integration/css-external/b.css

@@ -0,0 +1,3 @@
+.b {
+  color: red;
+}

packages/core/integration-tests/test/integration/css-node-modules/index.css

@@ -0,0 +1 @@
+@import 'npm:foo/foo.css';

packages/core/integration-tests/test/integration/image/reformat.js

@@ -1 +1,3 @@
-module.exports = require('./image.jpg?as=webp');
+import url from './image.jpg?as=webp';
+
+module.exports = url;

packages/core/integration-tests/test/javascript.js

@@ -4571,7 +4571,7 @@ describe('javascript', function() {
       name: 'BuildError',
       diagnostics: [
         {
-          message: 'Unknown pipeline: strange-pipeline.',
+          message: "Failed to resolve 'strange-pipeline:./b.js' from './a.js'",
           origin: '@parcel/core',
           codeFrames: [
             {
@@ -4592,6 +4592,10 @@ describe('javascript', function() {
             },
           ],
         },
+        {
+          message: "Unknown url scheme or pipeline 'strange-pipeline:'",
+          origin: '@parcel/resolver-default',
+        },
       ],
     });
   });

packages/core/integration-tests/test/transpilation.js

@@ -258,7 +258,7 @@ describe('transpilation', function() {
           .slice(2),
     );
     await outputFS.mkdirp(dir);
-    ncp(path.join(__dirname, '/integration/swc-helpers'), dir);
+    await ncp(path.join(__dirname, '/integration/swc-helpers'), dir);
     await bundle(path.join(dir, 'index.js'), {
       mode: 'production',
       inputFS: overlayFS,

packages/core/types/index.js

@@ -1449,22 +1449,29 @@ export type FileCreateInvalidation =
  * @section resolver
  */
 export type ResolveResult = {|
-  /** An absolute path to the file. */
+  /** An absolute path to the resolved file. */
   +filePath?: FilePath,
+  /** An optional named pipeline to use to compile the resolved file. */
   +pipeline?: ?string,
+  /** Query parameters to be used by transformers when compiling the resolved file. */
+  +query?: QueryParameters,
+  /** Whether the resolved file should be excluded from the build. */
   +isExcluded?: boolean,
+  /** Overrides the priority set on the dependency. */
   +priority?: DependencyPriority,
   /** Corresponds to BaseAsset's <code>sideEffects</code>. */
   +sideEffects?: boolean,
-  /** A resolver might want to resolve to a dummy, in this case <code>filePath</code> is rather "resolve from". */
+  /** The code of the resolved asset. If provided, this is used rather than reading the file from disk. */
   +code?: string,
   /** Whether this dependency can be deferred by Parcel itself (true by default). */
   +canDefer?: boolean,
   /** A resolver might return diagnostics to also run subsequent resolvers while still providing a reason why it failed. */
   +diagnostics?: Diagnostic | Array<Diagnostic>,
   /** Is spread (shallowly merged) onto the request's dependency.meta */
   +meta?: JSONObject,
+  /** A list of file paths or patterns that should invalidate the resolution if created. */
   +invalidateOnFileCreate?: Array<FileCreateInvalidation>,
+  /** A list of files that should invalidate the resolution if modified or deleted. */
   +invalidateOnFileChange?: Array<FilePath>,
 |};
 
@@ -1595,7 +1602,7 @@ export type Resolver = {|
     dependency: Dependency,
     options: PluginOptions,
     logger: PluginLogger,
-    filePath: FilePath,
+    specifier: FilePath,
     pipeline: ?string,
   |}): Async<?ResolveResult>,
 |};