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

@wyw-in-js/webpack-loader crashes when attempting to load files with queries #103

Open
fpapado opened this issue Sep 2, 2024 · 1 comment
Labels
bug 🐛 Issue is a confirmed bug bundler: webpack 📦 Issue is related to webpack bundler core team 🧬 Issues reserved for the core team

Comments

@fpapado
Copy link

fpapado commented Sep 2, 2024

Environment

  • wyw-in-js version: 0.5.4
  • custom processor: N/A
  • Bundler (+ version): [email protected]
  • Node.js version: v20.10.0
  • OS: macOS 14.6.1

Description

Webpack loaders typically rely on queries in a request path, in order to specify the loader. This is webpack's blessed way of configuration, and they provide a bunch of user-facing options around it.

For our case, we have a loader to load SVG elements as components. To disambiguate what loader is used, we use a resource query. We then want to style said component using linaria:

import { styled } from '@linaria/react';
import {Component as Arrow} from './arrow.svg?svgUse';

const StyledArrow = styled(Arrow)`
    color: turquoise;
`

...however, this crashes, when wyw-in-js attempts to read the resolved ./arrow.svg?svgUse verbatim from the filesystem.

ERROR in ./src/App.tsx
Module build failed (from ./node_modules/.pnpm/@[email protected][email protected][email protected]_@[email protected][email protected]_/node_modules/@wyw-in-js/webpack-loader/lib/index.js):
Error: ENOENT: no such file or directory, open '/redacted/wyw-in-js-with-webpack-loader-queries/src/assets/arrow.svg?svgUse'
    at readFileSync (node:fs:453:20)
    at Object.code (/redacted/wyw-in-js-with-webpack-loader-queries/node_modules/.pnpm/@[email protected][email protected]/node_modules/@wyw-in-js/transform/src/transform/Entrypoint.helpers.ts:177:44)

What (I think) happens under the hood

I have been investigating this issue, and here is what I've got:

  • When loading the code for a referenced module, @wyw-in-js/transform attempts to read referenced files from the filesystem (in Entrypoint.helpers.ts)
    • Some modules are "virtual"; they do not exist on the filesystem (this is more of a convention in Vite/Rollup)
    • Other modules use query parameters to specify a loader arrow.svg?svgUse
  • When wyw-in-js attempts to read the above from the filesystem, it crashes
  • Even in cases where wyw-in-js does read the file (e.g. by skipping the query parameter), I don't think it gets any useful information out of it.

This does not happen in the vite plugin. I imagine this is because vite/rollup plugins encode resolution semantics (the resolveId hook), so @wyw-in-js/transform only ever sees arrow.svg, after the svg plugin has resolved the path.

In contrast, webpack loaders typically rely on resource queries, and webpack loaders do not add a resolver. Perhaps the webpack plugin could manually strip out query parameters, to be on par with what happens in Vite?

Ideas for a fix

I would be more than happy to contribute a fix for this, if you can point me in the right direction 😊

The quick/hacky solution is to fall back if reading a file fails, and just assume that the referenced component is compatible. This would be done in transform's Entrypoint.helpers.js. This would work with any virtual modules. However, making this change in @wyw-in-js/transform feels like the wrong layering.

Another solution is to do something in the webpack loader, to augment the resolving strategy, in order to strip query parameters specifically. This would require exposing some resolution hook to/from transform. This approach will solve the query parameter issue for all webpack loaders, but not the virtual module issue (since virtual modules tend to be ad-hoc).

In either scenario, if wyw-in-js needs concrete information from the referenced file (e.g. if it is used as a selector in an interpolation), then I think those other codepaths will fail fast, same as now.

Reproducible Demo

  1. Visit https://github.com/fpapado/wyw-in-js-with-webpack-loader-queries
  2. You will need node and pnpm
  3. Run pnpm install
  4. Run pnpm build
  5. Expect to see the aforementioned error

Please let me know if there is any other information that I can provide 📝

@fpapado fpapado added bug report 🦗 Issue is probably a bug, but it needs to be checked needs: complete repro 🖥️ Issue need to have complete repro provided needs: triage 🏷 Issue needs to be checked and prioritized labels Sep 2, 2024
@github-actions github-actions bot added bundler: rollup 🗞️ Issue is related to rollup bundler bundler: webpack 📦 Issue is related to webpack bundler and removed needs: triage 🏷 Issue needs to be checked and prioritized labels Sep 2, 2024
@fpapado
Copy link
Author

fpapado commented Sep 7, 2024

Did some debug logging, to validate my assumptions.

DEBUG=wyw-in-js:transform* pnpm build

Using a vite plugin, which provides a resolveId hook the file gets resolved correctly on-disk, and wyw-in-js reads from it. It then ignores the result, since .svg is not in extensions, as expected.

  wyw-in-js:transform:00002#1:resolveImports@000009 next processing, result idx is 0 +0ms
  wyw-in-js:transform:00002#1 resolving 1 imports +0ms
  wyw-in-js:transform:00002#1 [async-resolve] ✅ ./assets/arrow.svg?svgUse ([ 'Component' ]) in /Users/fotis/svg-use/examples/vite-react/src/App.tsx -> /Users/fotis/svg-use/examples/vite-react/src/assets/arrow.svg +0ms
  wyw-in-js:transform:00002#1 resolved 1 imports +0ms
  wyw-in-js:transform:00002#1:transform@000008 next processing, result idx is 1 +0ms
  wyw-in-js:transform:00002#1:processImports@00000a next processing, result idx is 0 +0ms
  wyw-in-js:transform:00002#1 [createEntrypoint] /Users/fotis/svg-use/examples/vite-react/src/assets/arrow.svg is ignored. If you want it to be processed, you should add '.svg' to the "extensions" option. +0ms
  wyw-in-js:transform:00002#1->00003#1:source created /Users/fotis/svg-use/examples/vite-react/src/assets/arrow.svg ([ 'Component' ])
  wyw-in-js:transform:00002#1->00003#1:source [IGNORED] +0ms

Using the aforementioned webpack demo repository:

  wyw-in-js:transform:00002#1:resolveImports@000009 next processing, result idx is 0 +0ms
  wyw-in-js:transform:00002#1 resolving 1 imports +1ms
  wyw-in-js:transform:00002#1 [async-resolve] ✅ ./assets/arrow.svg?svgUse ([ 'Component' ]) in /Users/fotis/wyw-in-js-with-webpack-loader-queries/src/App.tsx -> /Users/fotis/wyw-in-js-with-webpack-loader-queries/src/assets/arrow.svg?svgUse +1ms
  wyw-in-js:transform:00002#1 resolved 1 imports +0ms
  wyw-in-js:transform:00002#1:transform@000008 next processing, result idx is 1 +0ms
  wyw-in-js:transform:00002#1:processImports@00000a next processing, result idx is 0 +0ms
  wyw-in-js:transform:00002#1 [createEntrypoint] /Users/fotis/wyw-in-js-with-webpack-loader-queries/src/assets/arrow.svg?svgUse is ignored. If you want it to be processed, you should add '.svg?svgUse' to the "extensions" option. +1ms
  wyw-in-js:transform:00002#1:processImports@00000a error processing, result idx is 0 +0ms
  wyw-in-js:transform:00002#1:processImports@00000a error Error: ENOENT: no such file or directory, open '/Users/fotis/wyw-in-js-with-webpack-loader-queries/src/assets/arrow.svg?svgUse'
    at readFileSync (node:fs:453:20)
    at Object.code (/Users/fotis/wyw-in-js-with-webpack-loader-queries/node_modules/.pnpm/@[email protected][email protected]/node_modules/@wyw-in-js/transform/src/transform/Entrypoint.helpers.ts:177:44)

I see two possible approaches, but please tell me if you know more:

The first, would be to augment the asyncResolve callback that the webpack-loader passes to transform, in order to strip the query parameters from the path. This would work with webpack's resource queries convention, which might be good enough.

diff --git a/packages/webpack-loader/src/index.ts b/packages/webpack-loader/src/index.ts
index 055d970..38e871f 100644
--- a/packages/webpack-loader/src/index.ts
+++ b/packages/webpack-loader/src/index.ts
@@ -73,8 +73,12 @@ const webpack5Loader: Loader = function webpack5LoaderPlugin(
     const context = path.isAbsolute(importer)
       ? path.dirname(importer)
       : path.join(process.cwd(), path.dirname(importer));
+
+    const tokenWithoutQuery = token.split('?', 1)[0];
+
     return new Promise((resolve, reject) => {
-      resolveModule(context, token, (err, result) => {
+      resolveModule(context, tokenWithoutQuery, (err, result) => {
         if (err) {
           reject(err);
         } else if (result) {

A second approach, is a change to transform: if wyw-in-js/transform knows that a path is ignored based on its extension, why is it reading the file contents? Would it be desirable to avoid reading the contents at all, or fail gracefully if it cannot read the contents? I could not figure out why the contents are important in those cases 💭

@layershifter layershifter added bug 🐛 Issue is a confirmed bug core team 🧬 Issues reserved for the core team and removed bug report 🦗 Issue is probably a bug, but it needs to be checked bundler: rollup 🗞️ Issue is related to rollup bundler needs: complete repro 🖥️ Issue need to have complete repro provided labels Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Issue is a confirmed bug bundler: webpack 📦 Issue is related to webpack bundler core team 🧬 Issues reserved for the core team
Projects
None yet
Development

No branches or pull requests

2 participants