fix: call resolveId hooks of custom plugins for css imports

[aleclarson]

Note: All plugins whose name matches vite:* are excluded from resolution of CSS imports. Any instances of @rollup/plugin-alias are also excluded (use resolve.alias instead).

[patak-dev]

Could you explain why we need to exclude internal plugins? If we are going to start having users plugins, should we directly the full pipeline and modify internal plugins where needed? If ours plugins don't work here, this may flag that users could also have issues with this change.

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
astro	✅ success
histoire	❌ failure
iles	✅ success
ladle	✅ success
laravel	✅ success
marko	❌ failure
nuxt-framework	✅ success
rakkas	✅ success
svelte	❌ failure
vite-plugin-ssr	❌ failure
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success
windicss	✅ success

[patak-dev]

For reference, the fails in ecosystem-ci are the same as with main.

[aleclarson]

Could you explain why we need to exclude internal plugins? If we are going to start having users plugins, should we directly the full pipeline and modify internal plugins where needed? If ours plugins don't work here, this may flag that users could also have issues with this change.

It's an optimization. None of Vite's internal plugins expect to receive CSS imports, so there's no benefit to using them (apart from vite:resolve which is always applied by createResolver).

[patak-dev]

Ah, gotcha. I think it would be good to measure if having this optimization gets us much. But let's leave it as is until we discuss the PR in a team meeting. Added it to the board now.

[aleclarson]

I think it would be good to measure if having this optimization gets us much.

That's a fair point. Is there a recommended way to benchmark the test suite?

Benchmarking isn't mentioned in the CONTRIBUTING.md guide (https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md).

[patak-dev]

@aleclarson we discussed the PR in today's team meeting, and this sounds good. Before merging, we need to check that enforce and order are being respected for user plugins.

[patak-dev]

Shouldn't this be inverted? The alias plugin is the first one in the regular pipeline.

[patak-dev]

Here it is tricky to properly order the plugins. The resolvePlugin should be injected in the same place where the regular resolve plugin is placed, no? Maybe the logic at 546 should generate two arrays, one for plugins before resolve, and one for the ones after it.

[patak-dev]

@aleclarson just a heads up in case you would like to push this feature for Vite 4, we're planning to release a beta soon and try to release the major around Dec 10. This is a feature we could include in a minor version later if not (just create the 4.1 milestone, we can move it there in that case)

[itsdouges]

This is a feature we could include in a minor version later if not (just create the 4.1 milestone, we can move it there in that case)

Hey folks is there a way we could get this in as a non-breaking change? For context I'm interested in this for our work on the Deno resolver plugin https://github.com/itsdouges/vite_plugin_deno_resolve

This looks to be a great first party solution to getting the optimizer using our resolver. What work is missing to get this merged?

[bluwy]

We need patak's review to be resolved and tests for this. One way we could make this non-breaking is an experimental option to enable this feature, which could be a good workaround as the Vite plugin can turn it on internally.

[hakubo]

Hey @patak-dev would you have some time soon(TM) to review this?

Thanks!

[aleclarson]

@hakubo He has reviewed it, and he raised some issues with the current state of the PR. This PR needs someone with time to push it forward (eg: write tests and resolve Patak's concerns).

[hakubo]

@aleclarson right! Sorry missed that. I see it is added to milestone 5 for now.

[bluwy]

FWIW I was able to workaround it by implementing this trick on the plugin side. Given most plugins don't need to tap into createResolver for CSS etc often, I'm slightly leaning towards the trick being a good enough solution so there's no potential perf loss.

[aleclarson]

I question whether config.createResolver(…) should even be a public API. Why isn't it createResolver(config, …) instead? It should at least be documented if monkey-patching it is seen as an acceptable maneuver. 😄

[emosheeep]

FWIW I was able to workaround it by implementing this trick on the plugin side.

I managed to do this originally using the following lines:

export function createTsAliasPlugin(): PluginOption {
  const tsResolvePlugin = tsconfigPaths({
    root: monoRoot,
    ignoreConfigErrors: true,
    loose: true, // to support resolve alias in scss files(any other non-script files)
  });
  return [
    tsResolvePlugin,
    {
      name: 'alias:style',
      config() {
        return {
          resolve: {
            alias: [
              {
                find: /(.*\.scss)$/,
                replacement: '$1',
                async customResolver(source, importer) {
                  if (importer && /\.scss/.test(importer)) {
                    // HACK: resolve alias in style files.
                    // @ts-ignore
                    return tsResolvePlugin.resolveId.apply(this, arguments);
                  }
                },
              },
            ],
          },
        };
      },
    },
  ];
}

But after I saw your implementation, I try to use it as follows:

export function createTsAliasPlugin(): PluginOption {
  return [
    tsResolvePlugin,
    {
      name: 'alias:style',
      configResolved(config) {
        patchCreateResolver(config, tsResolvePlugin);
      },
    },
  ];
}

It should work fine as I see, but actually not. Do you guys know what happened deep within it? @aleclarson @bluwy