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: externals bug in vite ssr, promote noExternal as first priority #6023

Closed
wants to merge 22 commits into from

Conversation

zhangyuang
Copy link
Contributor

@zhangyuang zhangyuang commented Dec 9, 2021

Description

Now, vite ssr external judge condition is not exact.
For example, vite will set default dependencies to ssr.externals, but easy to evaluated failed when someone use ui library like antd/vant

import { Button } from 'vant'

ssr.externals will be ['vant'].

But in general, these code will be written like

import { Button } from 'vant'
import 'vant/es/lib/button/style/index.js'

and style/index.js import css/less type file

$ cat node_modules/vant/es/button/style/index.js
import '../../style/base.css';
import '../../badge/index.css';
import '../../icon/index.css';
import '../../loading/index.css';
import '../index.css';%

The expected behavior is vant will be external but vant/es/lib/button/style/index.js cannot be externals need transformed, even if i set ssr.noExternal like

ssr: {
    external: ['vant'],
    noExternal: [/vant.*?style/]
 }

can't take effect because the judge condition is not exact.

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.

@zhangyuang zhangyuang marked this pull request as draft December 9, 2021 08:10
@zhangyuang zhangyuang marked this pull request as ready for review December 9, 2021 08:16
@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Dec 9, 2021
Shinigami92
Shinigami92 previously approved these changes Dec 9, 2021
@zhangyuang zhangyuang changed the title fix: externals bug in vite ssr fix: externals bug in vite ssr, promote noExternal as first priority Jan 4, 2022
@zhangyuang
Copy link
Contributor Author

zhangyuang commented Jan 6, 2022 via email

@benmccann
Copy link
Collaborator

At a high level, I'm not sure why there's two copies of the logic to use external / noExternal. It seems like the root cause of this issue is that shouldExternalizeForSSR and resolveSSRExternal are not in sync. It seems like resolveSSRExternal should use shouldExternalizeForSSR or that the result of resolveSSRExternal should be cached and we remove shouldExternalizeForSSR

@zhangyuang
Copy link
Contributor Author

At a high level, I'm not sure why there's two copies of the logic to use external / noExternal. It seems like the root cause of this issue is that shouldExternalizeForSSR and resolveSSRExternal are not in sync. It seems like resolveSSRExternal should use shouldExternalizeForSSR or that the result of resolveSSRExternal should be cached and we remove shouldExternalizeForSSR

resolveSSRExternal use rollup.createFilter to generate externals but it's not exactly when external and noExternal has partial intersection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants