fix: legacy no resolve asset urls

[poyoho]

Description

fix: #9530
fix: #9246

css-post plugin ignore processed vite asset flag.

VITE_ASSET and VITE_PUBLIC_ASSET urls are processed by the vite:asset plugin, don't call resolveAssetUrlsInCss here

If the css load by user normal plugins (vue plugin) will break the rule.

so we should resolveAssetUrlsInCss in legacy mode.

Additional context

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[poyoho]

https://github.com/poyoho/vite/blob/c1eb9b0d225e9688558af4a0145960bb93a5accd/packages/vite/src/node/plugins/css.ts#L566 what mean of inline css here. I try the html inline css and ?inline request is not to be here. 🤦

[sapphi-red]

It seems that it means "async css are inlined for legacy bundle" 🤔
poyoho@940d483

[poyoho]

IIUC, now is ok 😁

[poyoho]

https://github.com/poyoho/vite/blob/2d7812af07f872d21e4c4288b1fbdb66795133c7/packages/vite/src/node/plugins/css.ts#L475 this variable is seems must be true here so I remove this logic for publicPath format.

[sapphi-red]

Yes, rollup seems to avoid generating module argument like that.
https://github.com/rollup/rollup/blob/a8647dac0fe46c86183be8596ef7de25bc5b4e4b/src/finalisers/system.ts#L39-L43

For legacy chunks maybe we can add import.meta.url; in transform hook to force rollup to generate module argument. It's a hacky solution though...

[patak-dev]

module is the second argument of System.register callback but rollup outputs function (exports) { instead of function (exports, module) {, when module isn't used.

Oh, I really didn't expect this one, sorry I approved this one 👀.
This should also be affecting other places, no? We also use renderChunk in assets, so we should be having the same issue for System.js there. I hope we can start using rollup infra soon to avoid these problems. It doesn't seem easy to solve. Should we inject import.meta.url in https://rollupjs.org/guide/en/#outputintrooutputoutro ? Edit: ya, your idea of using the transform hook is better.

[sapphi-red]

This should also be affecting other places, no? We also use renderChunk in assets, so we should be having the same issue for System.js there.

Ah, yes. We'll need to do a similar thing there, too.

I hope we can start using rollup infra soon to avoid these problems.

Yeah, I hope it but I don't have good idea to use it here. We are using rollup's chunk generating mechanism to collect css into chunks. So we need to use renderChunk. On the other hand, we need to use hooks before renderChunk to use rollup's import.meta.url transform.

[poyoho]

how about arguments[1].meta.url 👀 arguments can run in https://caniuse.com/?search=arguments. If function is not allow function would had arguments

[sapphi-red]

arguments will have a different value if it's inside a different function. So I guess it's not safe to use.

System.register('', function () {
  function bar () {
    console.log(arguments[1].meta.url) // here
  }
  bar()
})

[poyoho]

For legacy chunks maybe we can add import.meta.url; in transform hook to force rollup to generate module argument. It's a hacky solution though...

It very hard to inject the import.meta.url in raw code. Such as vue component the js will in return in .vue file no .js file. we only can add the import.meta.url; in maybe js file

[sapphi-red]

I was thinking of something like:

transform(code) {
  return `${code};import.meta.url;`
}

Not injecting in the actual position. (rollup might remove this with tree-shaking and I haven't tested this)

[poyoho]

Not injecting in the actual position. (rollup might remove this with tree-shaking and I haven't tested this)

may by inject the import.meta.url in toOutputFilePathInString make sure rollup would generate module params?

But somewhere using toOutputFilePathInString in renderChunk.

[poyoho]

emm... In this PR, must be hoist the css inject code in the global scoped. So I think arguments is good for the sence.

About https://github.com/rollup/rollup/blob/a8647dac0fe46c86183be8596ef7de25bc5b4e4b/src/finalisers/system.ts#L39-L43. I think the accessedGlobals will collect the global accessed in the scoped, and this is not configureable. It seems we only can hack it. But I think this is not safe to use.

How about force replace System.register([], function(export)) to System.register([], function(export, module)) like force inject the css legacy code in the SystemJS wrap.

[sapphi-red]

How about force replace System.register([], function(export)) to System.register([], function(export, module)) like force inject the css legacy code in the SystemJS wrap.

This one sounds good to me.

[patak-dev]

Yes, that is a smart trick @poyoho!

[sapphi-red]

Thanks for tackling this!

[patak-dev]

Seconded, amazing how much complexity was unveiled by this issue. Thanks for pushing it til the end @poyoho. And your reviews on this one were great @sapphi-red

[poyoho]

This PR deepen my understanding of vite. Thanks for everyone's review