[plugin-dynamic-import-vars] non-normalized paths no longer work

[bearfriend]

• Rollup Plugin Name: plugin-dynamic-import-vars
• Rollup Plugin Version: 2.1.4
• Rollup Version: *
• Operating System (or Browser): *
• Node Version: *
• Link to reproduction: https://stackblitz.com/edit/rollup-repro-blsfye?file=src%2Fmain.js

Expected Behavior

Imports passed non-normalized paths resolve the bundled file

Actual Behavior

Imports passed non-normalized paths produce a Unknown variable dynamic import error

Additional Information

#1780 breaks any code that doesn't build normalized paths because tinyglobby always outputs normalized glob paths but the plugin doesn't touch the original code.

Given this code:

import(`./../${myFile}.js`)

tinyglobby produces ../*.js -> ../myFile.js-> case '../myFile.js', which now doesn't match ./../myFile.js.

I think the only reasonable options are:

1. Revert
2. Ask tinyglobby to make normalization optional (will take time and maybe isn't even feasible/desired)
3. Normalize the path inside __variableDynamicImportRuntime__ before the switch with path-normalize

[shellscape]

@benmccann looks like you missed an edge case here. please take a look.

[benmccann]

CC @SuperchupuDev it looks like there's a behavior difference between fast-glob and tinyglobby that has broken things here

[shellscape]

@benmccann if it's a upstream issue reverting the change until we can get the tinyglobby issue resolved is probably the wise play here.

[SuperchupuDev]

tinyglobby intentionally doesn't change the format of the glob results depending on the pattern provided for better consistency and also due to the way it works (tinyglobby doesn't transform results except on very specific circumstances). i believe vite had to apply a workaround here to make it work, but in theory using path.normalize here should work

[bearfriend]

tinyglobby intentionally doesn't change the format of the glob results depending on the pattern provided

What do you mean by "depending on the pattern"? It seems the pattern itself is normalized before generating results, here, from processPattern.

[SuperchupuDev]

that normalizes the pattern to match, not the way the results from the directory crawler are generated. the only case where the patterns can modify the result is if any of the patterns start with ../, but that's just because the crawler can't find any files outside the root without a workaround

the crawler library generates a result that's always the exact same no matter the patterns, and each possible result that's generated is filtered by the (normalized) patterns

if you have a directory that has a/a.txt for example, no matter what pattern you use, the crawler will see that file as a/a.txt when filtering and producing results

[bearfriend]

the crawler library generates a result that's always the exact same no matter the patterns, and each possible result is generated is filtered by the (normalized) patterns

So then the issue is with the crawler then. I don't think it's actually an issue, though. Certainly not from its perspective. It might be best to just normalize the paths at runtime in the plugin.

[SuperchupuDev]

IIRC vite fixed this by appending ./ to all the results if the pattern starts with ./

[benmccann]

Just FYI I am very heads down prepping for Svelte Summit this week. If someone is able to look at this issue that would be great. Otherwise I'll take a look next week

[shellscape]

Good to know. We'll give it a day to see if anyone can step in and if not I'll revert.

[bearfriend]

@SuperchupuDev That may have worked for them because they could ultimately control what the inputs were, but here it could be anything so there's no static approach that will guarantee we reconstruct the original path. For instance:

// you can imagine this path being constructed by a series of `concats`
import('./${myHost}/./${myProject}/./${myComponent}.js`')

turns into ./*/*/*.js, which results in ./my-host/my-project/my-component.js, so the only way I see to make this work is to run it through a port of the posix normalize method at runtime.

[bearfriend]

If we're ok adding path-normalize I can do that work now.

[shellscape]

@bearfriend not opposed to it in the plugin. we'll want a regression test for this car as well.

[bearfriend]

@shellscape I believe I have something working that I'm ready to open a PR for, ~but I'm having an issue when trying to commit. Dependencies are updated, but I don't use pnpm so maybe I'm missing something.~

Edit: Got it sorted. I must have had some junk in node_modules from an old branch and maybe accidentally using npm to install.

✔ Preparing...
⚠ Running tasks...
  ❯ Running tasks for *.{ts,js}
    ✖ eslint --cache --fix [FAILED]
  ↓ No staged files match **/(package|tsconfig(.*)?).json [SKIPPED]
  ↓ No staged files match (pnpm-workspace|.github/**/*).{yml,yaml} [SKIPPED]
  ↓ No staged files match ((.github/**/*)|(README|CHANGELOG)|(**/(README|CHANGELOG))).md [SKIPPED]
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up...

✖ eslint --cache --fix:

Oops! Something went wrong! :(

ESLint: 8.57.1

TypeError: prettier.resolveConfig.sync is not a function

[shellscape]

Making the decision to revert the changes. The only tangible benefit was a reduction of dependencies, but the juice isn't worth the squeeze.