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

feat(ssr): tolerate circular imports #3950

Merged
merged 13 commits into from
Aug 2, 2021

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Jun 24, 2021

Description

This PR makes SSR imports work like Node.js in the following way:

Modules that depend on each other will never receive an undefined exports. Instead, the module that is first to import the other module will wait for the other module to be instantiated. When the other module imports the first module, it receives the incomplete exports object, which will be completed when the first module is done being instantiated.

To accomplish this behavior, we need to load SSR dependencies one at a time. This ensures a predictable import order. Before this PR, the import order is non-deterministic, as it depends on the depth of the dependency tree plus the transformation time per module.

This PR also avoids infinite hangs caused by a dependency having its ssrLoadModule promise awaited without first checking if one of its dependencies is within the urlStack.

As of 0333312, this PR also allows the "other module" to access the partial exports of the "first module". For example, if export { foo } from "./foo" comes before export { bar } from "./bar" in the ./index.js module, then ./bar.js will still be able to import { foo } from "./index" and use it before ./index.js has finished being instantiated.

Todo

  • Split out the playground/ssr-react changes into a new playground ssr-circular-import

Additional context

Fixes #2258
Fixes #2491


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.

@aleclarson aleclarson added feat: ssr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels Jun 24, 2021
@raythurnvoid
Copy link
Contributor

raythurnvoid commented Jun 25, 2021

@aleclarson i've brought my improvement to your branch: aleclarson#2

in can still be improved but it works on the project i'm working at the moment

@aleclarson
Copy link
Member Author

@raythurnevoid Thanks for the test case. I've solved it in a better way, see here: 0333312

@raythurnvoid
Copy link
Contributor

@raythurnevoid Thanks for the test case. I've solved it in a better way, see here: 0333312

I can confirm it is working on the project i'm working too

@aleclarson aleclarson force-pushed the feat/ssr-circular-import branch from 22b88e7 to 840d020 Compare June 27, 2021 20:36
raythurnvoid
raythurnvoid previously approved these changes Jun 27, 2021
@patak-dev
Copy link
Member

@aleclarson would you resolve the conflicts? Is this one ready to be discussed with the team?

@Shinigami92
Copy link
Member

Should @brillout have a look?

@brillout
Copy link
Contributor

brillout commented Jul 16, 2021

@Shinigami92 LGTM (Although my review is superficial and I didn't dig that much)

@aleclarson Neat neat - one thing though: how about we create a new playground instead of populating playground/ssr-react/? I often point people to playground/ssr-react and playground/ssr-vue to show them Vite's native SSR API. Would be good for them if we keep these playgrounds minimal. Thoughts?

@LydiaF Hope this makes you re-consider Vite hehe.

brillout
brillout previously approved these changes Jul 16, 2021
@LydiaF

This comment has been minimized.

@brillout

This comment has been minimized.

@aleclarson
Copy link
Member Author

how about we create a new playground instead of populating playground/ssr-react/? I often point people to playground/ssr-react and playground/ssr-vue to show them Vite's native SSR API. Would be good for them if we keep these playgrounds minimal. Thoughts?

I agree. The tests were contributed by others. If someone can create the new playground, it would be appreciated. 👍

aleclarson and others added 3 commits July 29, 2021 16:52
If dependencies are not loaded one at a time, the load order depends on the depth of the dependency tree plus the transformation time per module. When forced to load serially, the load orders depends on the order of import declarations, which is how NodeJS works too.
@aleclarson aleclarson force-pushed the feat/ssr-circular-import branch from c0f4983 to 49aee2c Compare July 29, 2021 20:53
aleclarson added a commit to aleclarson/vite that referenced this pull request Jul 30, 2021
antfu
antfu previously approved these changes Aug 2, 2021
Shinigami92
Shinigami92 previously approved these changes Aug 2, 2021
packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated Show resolved Hide resolved
@aleclarson aleclarson dismissed stale reviews from Shinigami92 and antfu via 6bf666f August 2, 2021 17:20
@patak-dev patak-dev requested a review from antfu August 2, 2021 17:53
@aleclarson aleclarson merged commit 69f91a1 into vitejs:main Aug 2, 2021
@aleclarson aleclarson deleted the feat/ssr-circular-import branch August 2, 2021 19:26
aleclarson added a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Co-authored-by: Greg Fairbanks <[email protected]>
Co-authored-by: Ray Thurne Void <[email protected]>
@rtsao
Copy link
Contributor

rtsao commented Jun 27, 2023

It seems like there is still possible issues with circular imports and SSR that can lead to intermittent hanging: #11468

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Circular dependencies in SSR not detected and causes indefinite hang SSR support circular dependency
9 participants