fix: enforce generating module param when needed in relative url resolving (fixes #9807)

[Tal500]

More details on the issue it fixes #9807 .

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.

[sapphi-red]

Thank you for the PR!

We are doing a similar thing in packages/vite/src/node/plugins/completeSystemWrap.ts.
I missed this case when checking (#9507 (comment)).

Maybe just updating this regex will fix your issue?

vite/packages/vite/src/node/plugins/completeSystemWrap.ts

Line 7 in b2c0ee0

const SystemJSWrapRE = /System.register\(.*\((exports)\)/g

[Tal500]

Thank you for the PR!

We are doing a similar thing in packages/vite/src/node/plugins/completeSystemWrap.ts. I missed this case when checking (#9507 (comment)).

Maybe just updating this regex will fix your issue?

vite/packages/vite/src/node/plugins/completeSystemWrap.ts

Line 7 in b2c0ee0

const SystemJSWrapRE = /System.register\(.*\((exports)\)/g

Hi!

What are you suggesting to do then? I'm not an expert in vite plugins, but this is what I understand:
The regex you mentioned is fine, it catches my use case (thought unsafe in my opinion, since the expresion (export) might appear later, not in the function deceleration), but is done at an early stage (in pre): (in

vite/packages/vite/src/node/build.ts

Lines 306 to 308 in 757a92f

	return {
	pre: [
	completeSystemWrapPlugin(),

)

I also think it's a tiny waste to force declaring a parameter that might not be used at all.

The problem I'm facing is this: In some vite internal plugins, this can happen in this order:

1. Rollup compiles the asset in system format.
2. Then, in stage renderChunk, the plugins get the raw compiled Rollup code, and can process it more.
3. They sometimes might inject new code, that sometimes depends on the definition of module.
4. If the raw code from 1 didn't use the module definition, Rollup chooses smartly not to declare this parameter. This can conflict the behavior of Vite plugins that can inject new code in 3 that uses the definition of module.

So I think the plugin of completeSystemWrapPlugin is executed before other plugins implementing renderChunk.

The approach I have in this PR is:

1. Whenever creating a new code that might need the definition of module, you need to return not only the new code, but also an indicator telling whether it uses the module definition (I called this indicator needModuleParam).
2. The plugin using it in renderChunk, will just use my newly created function ensureHavingSystemJSModuleParam for forcing the signature to be function (export, module) { /* ... */ }, and only if the definition of module is really needed.

[Tal500]

Thank you for the PR!

We are doing a similar thing in packages/vite/src/node/plugins/completeSystemWrap.ts. I missed this case when checking (#9507 (comment)).

Maybe just updating this regex will fix your issue?

vite/packages/vite/src/node/plugins/completeSystemWrap.ts

Line 7 in b2c0ee0

const SystemJSWrapRE = /System.register\(.*\((exports)\)/g

I post PR that solves my problem in your way, although I'm not sure it's good to do this way, but I do understand the safety concerns in this giant project.
See PR #9821

If the other PR is accepted, this PR can be closed.

[poyoho]

Thanks for PR. This PR can fix tiny waste to force declaring a parameter.

toOutputFilePathInString generate code maybe need module params. And toOutputFilePathInString always call in renderChunk. So we can save the needModuleParam state in chunk and replace it in

vite/packages/vite/src/node/plugins/completeSystemWrap.ts

Line 7 in b2c0ee0

const SystemJSWrapRE = /System.register\(.*\((exports)\)/g

I feel that can reduce many parameters that need to be added for state transfer

Another chose

I think we can do this in completeSystemWrap.

Pre renderChunk plugin collect what chunk needModuleParam

Post renderChunk plugin replace it by needModuleParam

[sapphi-red]

Yes, this PR's approach can reduce the parameter.

But

• it requires much more code
• it only reduces about two to five bytes for each chunk when minified (, and the argument name)

. So I thought it does not justify the complexity.

[patak-dev]

Agreed that the increased complexity doesn't justify this approach, let's go with #9821
@Tal500, I just created a v3.0 branch from the last release point (we are now preparing 3.1, beta to be out soon), would you like to cherry-pick the commit to backport this fix to the new branch? https://github.com/vitejs/vite/tree/v3.0

[Tal500]

Agreed that the increased complexity doesn't justify this approach, let's go with #9821 @Tal500, I just created a v3.0 branch from the last release point (we are now preparing 3.1, beta to be out soon), would you like to cherry-pick the commit to backport this fix to the new branch? https://github.com/vitejs/vite/tree/v3.0

Tnx! But I didn't understand the question...

[patak-dev]

@Tal500 don't worry, I'll do it before we release 3.0.10. I was talking about cherry-picking this commit (1ee0364) into the v3 branch. main is currently for the next minor (3.1), and this is an important enough fix to justify a new patch in 3.0