Skip to content

Allow plugins to overwrite built-in loaders#522

Merged
marvinhagemeister merged 1 commit intomainfrom
default-loaders
Apr 2, 2021
Merged

Allow plugins to overwrite built-in loaders#522
marvinhagemeister merged 1 commit intomainfrom
default-loaders

Conversation

@marvinhagemeister
Copy link
Member

@marvinhagemeister marvinhagemeister commented Apr 2, 2021

With this change default loaders are only added if no other prefix is present already in the path id.

I've tried a few other alternatives to the changes here, but they all end up working around limitations in rollup's plugin system and make user plugins pretty verbose to write. The gist of the problem is that with custom plugins we'll have to deal with recursive resolution early on.

The way that this usually occurs is that a custom plugin strips their prefix and passes the stripped id on to this.resolve. The plugins resolved by that have no knowledge about the original path and just see ./foo.json and add their prefix, because they assume that this is the full import specifier. This leads to errors like the one outlined in #449 where my-json:./foo.json will be turned into my-json:json:./foo.json. The changes in this PR ensure that my-json:./foo.json stays that way.

We essentially split up resolution and applying default loaders into two distinctive concerns. The plugins itself should only match when their prefix is present. The defaultLoader plugin ensures that "raw" specifiers like ./foo.json are turned into json:./foo.json from where we can just reused the existing resolution semantics.

Fixes #449

@changeset-bot
Copy link

changeset-bot bot commented Apr 2, 2021

🦋 Changeset detected

Latest commit: 31c83e7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wmr Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

With this change default loaders are only added if no other prefix
is present already in the path id
@github-actions
Copy link
Contributor

github-actions bot commented Apr 2, 2021

Size Change: -47 B (0%)

Total Size: 733 kB

Filename Size Change
packages/wmr/wmr.cjs 700 kB -47 B (0%)
ℹ️ View Unchanged
Filename Size Change
examples/demo/dist/about/index.html 667 B 0 B
examples/demo/dist/assets/Calendar.********.css 702 B 0 B
examples/demo/dist/assets/style.********.css 386 B 0 B
examples/demo/dist/chunks/class-fields.********.js 200 B 0 B
examples/demo/dist/chunks/compat.********.js 15.3 kB 0 B
examples/demo/dist/chunks/index.********.js 199 B 0 B
examples/demo/dist/chunks/json.********.js 235 B 0 B
examples/demo/dist/chunks/prerender.********.js 2.41 kB 0 B
examples/demo/dist/class-fields/index.html 646 B 0 B
examples/demo/dist/compat/index.html 1.5 kB 0 B
examples/demo/dist/env/index.html 722 B 0 B
examples/demo/dist/error/index.html 656 B 0 B
examples/demo/dist/files/index.html 688 B 0 B
examples/demo/dist/index.********.js 7.23 kB 0 B
examples/demo/dist/index.html 714 B 0 B
examples/demo/dist/json/index.html 666 B 0 B
examples/demo/dist/lazy-and-late/index.html 668 B 0 B

compressed-size-action

@marvinhagemeister marvinhagemeister merged commit c4728db into main Apr 2, 2021
@marvinhagemeister marvinhagemeister deleted the default-loaders branch April 2, 2021 11:54
This was referenced Apr 2, 2021
@danielweck
Copy link

I am getting uncaught errors ERR_HTTP_HEADERS_SENT Cannot set headers after they are sent to the client since around the time this PR was merge into the main branch. I am not 100% this PR is responsible though. Investigating...

(node:59869) UnhandledPromiseRejectionWarning: Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at ServerResponse.setHeader (_http_outgoing.js:558:11)
    at ServerResponse.c.writeHead (/PATH_TO/node_modules/wmr/wmr.cjs:2:2663686)
    at WZe (/PATH_TO/node_modules/wmr/wmr.cjs:2:2660743)
    at Array.<anonymous> (/PATH_TO/node_modules/wmr/wmr.cjs:2:2662442)
    at u (/PATH_TO/node_modules/wmr/wmr.cjs:2:2648010)
    at l (/PATH_TO/node_modules/wmr/wmr.cjs:2:2647978)
    at Array.<anonymous> (/PATH_TO/node_modules/wmr/wmr.cjs:2:2662478)
    at u (/PATH_TO/node_modules/wmr/wmr.cjs:2:2648010)
    at l (/PATH_TO/node_modules/wmr/wmr.cjs:2:2647978)
    at Array.<anonymous> (/PATH_TO/node_modules/wmr/wmr.cjs:2:2801453)
(node:59869) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 30)

@danielweck
Copy link

Okay, the above exception is thrown whenever I hard-refresh the page with CMD+R (any route, lazy included).

@danielweck
Copy link

I tested this PR. The main goal is indeed achieved, and as demonstrated by your integration tests, this closes #449
Well done 👍 :)

However, this PR breaks "nested absolute path import" as described here:
#449 (comment)

...I will be searching for a workaround for this, as it is an important feature in a codebase where relative paths such as ../../../stuff.js are a pain. I will let you know ;)

@danielweck
Copy link

danielweck commented Apr 2, 2021

Ah, interesting: the "nested absolute path import" only occurs with JSON files, not SVG. I will post a new issue once I've narrowed this down.

  wmr:middleware  /~/pages/test.json -> json:~/pages/test.json file: json:/PATH_TO/public/~/pages/test.json
  middleware:npm  "preact-iso/hydrate"
404 /@json/~/pages/test.json - File not found
  at  (:-1:-1)

@danielweck
Copy link

danielweck commented Apr 2, 2021

Okay, I found a workaround for JSON files, using the same trick I used before for SVG files :)

In the text: plugin's resolveId(id, importer):

if (config.mode === 'start') id = `${id}${'\0'}`;

..and in load(id):

if (config.mode === 'start') id = id.replace(/\0$/, '');

I am filing a new issue.

@marvinhagemeister
Copy link
Member Author

@danielweck thanks for giving the fix a go 👍 The path aliasing thing is next on my list and I think I have an idea of where things get weird.

@danielweck
Copy link

I filed this issue: #524

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Userland plugin doesn't work if it uses the the same file extension as a built-in plugin, with a different import prefix

3 participants