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(ssr): improved dependency resolution #5220

Closed
wants to merge 3 commits into from

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Oct 7, 2021

This PR affects SSR only.

  • Support resolveId hooks that coerce bare imports into local module paths.
    For example, the vite-tsconfig-paths plugin.

    • How? After a bare import is resolved by vite:resolve, check if the resolved ID is an absolute path that exists in the project root. If it is, avoid marking it as external (for SSR builds) and return the absolute path.
  • Respect ssr.noExternal inside ssrImport

    • Add the createSSRExternalsFilter function, which wraps createFilter from @rollup/plugin-utils
    • Avoid coercing bare imports into absolute paths during import analysis
      • After a bare import is resolved by vite:resolve, check if the resolved ID is an absolute path that exists in the project root. If not, preserve the unresolved id so the ssr.noExternal check in ssrImport can work properly.
  • Note that ssrImport will never receive the following specifiers (after this PR is merged):

    • relative path
    • path with /@id/ prefix
    • path with /@fs/ prefix
  • Use moduleGraph.getModuleByUrl instead of accessing urlToModuleMap directly, so that bare imports added to ssr.noExternal are normalized first. Otherwise, the module will always be undefined.

    • This issue was reported in Discord here, and I've also experienced it myself.
  • Remove the need for unwrapId inside ssrImport

    • How? Stop prepending /@id/ to bare imports inside vite:import-analysis

- never prepend /@id/ to bare imports
- preserve bare imports rather than convert them to absolute paths which /@fs/ prefix

Note that `ssrImport` will never receive the following specifiers (after this PR is merged):

  - relative path
  - path with /@id/ prefix
  - path with /@fs/ prefix
@aleclarson aleclarson added the p4-important Violate documented behavior or significantly improves performance (priority) label Oct 7, 2021
@Shinigami92 Shinigami92 marked this pull request as draft October 8, 2021 07:09
@yyx990803
Copy link
Member

Seems there's one test failing - looks good at a first glance but ping me after we fix the tests.

@patak-dev
Copy link
Member

@aleclarson I'm wondering how we could merge the improvements in this PR. Before the conflicts, there were test fails and that is why it stalled. Since there are several changes in the PR, would it make sense to create smaller PRs so it is easier to make the CI happy (at least for some of them) and we can get them in?

@aleclarson
Copy link
Member Author

Sorry the radio silence. Quick update from me: I won't be able to push this forward, since my time is limited and I'm not currently using Vite SSR. Hopefully someone can fork this PR and rebase / fix the tests. 🤞

@aleclarson aleclarson closed this Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants