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

fix(v6): fix maximum call stack error in ModuleRunner.isCurcularImport #17740

Merged

Conversation

hi-ogawa
Copy link
Collaborator

Description

While testing v6 https://pkg.pr.new/vite@4e81092 in hi-ogawa/vite-plugins#297, I'm seeing ModuleRunner.isCurcularImport maximum call stack error when dynamic importing a server action.

I added a test case and verified the fix. (I also patched locally on my project and verified it's fixed).

Copy link

stackblitz bot commented Jul 23, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@hi-ogawa hi-ogawa marked this pull request as ready for review July 23, 2024 00:42
@@ -162,8 +162,16 @@ export class ModuleRunner {
return false
}

private isCurcularImport(importers: Set<string>, moduleId: string) {
private isCurcularImport(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hi-ogawa would you also update the name to be isCircularImport ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, didn't notice. There's one more above isCurcularModule. I fixed that too.

@patak-dev
Copy link
Member

Apart from taking the opportunity to do the rename, this looks good to me. cc @sheremet-va so you're aware. @hi-ogawa I think you can merge it directly after you update the name

@hi-ogawa hi-ogawa merged commit 4b1080a into vitejs:v6/environment-api Jul 23, 2024
7 checks passed
@hi-ogawa hi-ogawa deleted the fix-isCurcularImport-infinite-loop branch July 23, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants