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: respect explicitily external/noExternal config #8983

Merged
merged 2 commits into from Jul 8, 2022

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Jul 8, 2022

Description

Fixes #8974 when vite-plugin-ssr is explicitely configured as ssr.external

@brillout following the previous logic, it looks like defining a dependency as noExternal wasn't affecting its internal dependencies. Is this what is expected?

Imagine if vite-plugin-ssr is ESM, when you mark vikepress as noExternal, do you expect vite-plugin-ssr to also be noExternal?

This is how optimizeDeps.exclude works, the whole dep tree is excluded according to @bluwy. That is why we need a optimizeDeps.include: 'entry > sub' if we want to escape that.

We could make noExternal work like optimizeDeps.exclude, but I think at least for v3, just making everything external by default is ok. This means that you shouldn't need to explicitly configure vite-plugin-ssr as external, as noExternal only affects a single node imported file and not the imported modules from it. Let me know if this sounds ok to you too.

The current code, once this PR is merged should already work like this. But there is another bug at play that prevents vite-plugin-ssr to be detected as external. Looks like this tryNodeResolve call:

return !!tryNodeResolve(

is failing. @bluwy saw that it is working if vite-plugin-ssr is also installed as an entry.

I'm working on a refactoring, I think we should directly check do the external/noExternal logic inside tryNodeResolve. Pushing this PR to keep the conversation going in the mean time.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@netlify
Copy link

netlify bot commented Jul 8, 2022

Deploy Preview for vite-docs-main canceled.

Name Link
🔨 Latest commit 1699ab8
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62c7e81ac262430008690b5a

@bluwy
Copy link
Member

bluwy commented Jul 8, 2022

We could make noExternal work like optimizeDeps.exclude, but I think at least for v3, just making everything external by default is ok. This means that you shouldn't need to explicitly configure vite-plugin-ssr as external, as noExternal only affects a single node imported file and not the imported modules from it. Let me know if this sounds ok to you too.

I think we should make noExternal work like optimizeDeps.exclude now though, so if we noExternal vikepress, vite-plugin-ssr would be noExternal too. Because after building, the output code can't import vite-plugin-ssr as it's not a direct dependency.

@patak-dev
Copy link
Member Author

patak-dev commented Jul 8, 2022

I think we should make noExternal work like optimizeDeps.exclude now though, so if we noExternal vikepress, vite-plugin-ssr would be noExternal too. Because after building, the output code can't import vite-plugin-ssr as it's not a direct dependency.

So vite-plugin-ssr needs to be a peer dep of vikepress to be able to externalize it. I think it isn't bad that it is external by default even if vikepress is marked as no external in this case, as Vite should prefer external as possible. If we have an installed dep, better it be external unless explicitly marked as noExternal

@patak-dev patak-dev marked this pull request as ready for review July 8, 2022 07:33
@brillout
Copy link
Contributor

brillout commented Jul 8, 2022

On a high-level I think we should only noExternal VikePress while vite-plugin-ssr is externalized. Because we want Vite to externalize as many SSR dependencies as possible. That's what I've have been pushing for with e.g. "always externalize SSR deps by default". CC @benmccann which as been as well pushing for such default.

Now @bluwy has a point that if VikePress is noExternal then VikePress's code lives in node_modules/.vite/[edit] dist/ and therefore cannot import its direct dependencies (e.g. import 'vite-plugin-ssr') when using pnpm without shamefully-hoist=true.

One way we can solve this is to resolve the import statements in node_modules/.vite/[edit] dist/: instead of having import 'vite-plugin-ssr' we would have import '../../node_modules/.pnpm/vikepress@0.0.23_biqbaboplfbrettd7655fr4n2y/node_modules/vite-plugin-ssr'.

CC @benmccann SvelteKit also have the situation that some SSR dependencies need to be noExternal. What do you think?

@patak-dev
Copy link
Member Author

One way we can solve this is to resolve the import statements in node_modules/.vite/: instead of having import 'vite-plugin-ssr' we would have import '../.pnpm/vikepress@0.0.23_biqbaboplfbrettd7655fr4n2y/node_modules/vite-plugin-ssr'.

Why don't make 'vite-plugin-ssr' a peer dependency? Seems like something users should install in their projects. VikePress code doesn't live in node_modules/.vite/ as it won't be optimized because you want to use Vite features on it. VikePress is noExternal and ssr.exclude so the code will live in your dist folder.

@brillout
Copy link
Contributor

brillout commented Jul 8, 2022

Ah, yes, sorry it lives in dist/ not .node_modules/.vite/. The problem is the same though.

For the vite-plugin-ssr dependency, yes, having it as a peer dependency would be an option. But that's not the case for other dependencies: for example the twemoji dep should not be a peer dep.

@patak-dev
Copy link
Member Author

For the vite-plugin-ssr dependency, yes, having it as a peer dependency would be an option. But that's not the case for other dependencies: for example the twemoji dep should not be a peer dep.

Why twemoji works as external in v2 if it isn't installed? v2 will externalize it, but then it could fail depending on the used pm, no? v3 is now more strict here, you could place twemoji in external and after this PR it will be externalized even if it isn't installed and you'll need to use a setup that supports that. Changing the paths would count as a feature request, no?
Are we ok with the more strict check we have now in v3? v3 is making more entries external by default, but internal deps are now noExternal if not installed. This is a departure from how things worked in v2

@bluwy
Copy link
Member

bluwy commented Jul 8, 2022

I'm not really sure about rewriting imports for externalized deps since it makes the output code not portable, and agree with patak that we could discuss this separately since this feature doesn't exist in Vite 2 too.

@patak-dev
Copy link
Member Author

Another note, in v2, twemoji would have worked in dev SSR depending on your setup. In v3 it is not going to externalize it if it isn't installed also for dev SSR.

@brillout
Copy link
Contributor

brillout commented Jul 8, 2022

I just checked and v2 actually does noExternalize twemoji. So it works regardless of the pm.

In v3 it is not going to externalize it if it isn't installed

👍 Seems like a reasonable default.

Are we ok with the more strict check we have now in v3?

From my side, yes. (VikePress is a low-key project anyways.)

But I think we should check with the SvelteKit folks. (Svelte libraries can contain .svelte source files; they must use ssr.noExternal as well.)

we could discuss this separately since this feature doesn't exist in Vite 2 too

👍 #8991.

@patak-dev patak-dev mentioned this pull request Jul 8, 2022
4 tasks
@patak-dev
Copy link
Member Author

@brillout running brillout/vite-3-legacy-build-bug against this PR properly externalizes vite-plugin-ssr, and then we get:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'vite-plugin-ssr' imported from /Users/patak/test/vite-3-legacy-build-bug/dist/server/assets/chunk-93b94dab.js

Is my understanding correct that after this PR is merged and you update vikepress, CI should be green for vite-plugin-ssr?

Let's merge this PR as we should respect users explicit external/noExternal configs independently of any other changes we will propose later.

About this, we may still have a bug lurking, but we'll need a different issue and reproduction if this is still a problem:

Note that the reproduction is green in vite-plugin-ssr's CI (the reproduction is a subset of the CI). The difference with the CI is that vite-plugin-ssr is symlinked in the CI. So I'm suspecting some kind of node_modules if-branch to be the culprit.

@patak-dev patak-dev merged commit e369880 into main Jul 8, 2022
@patak-dev patak-dev deleted the fix/ssr-externals-logic branch July 8, 2022 10:20
@brillout
Copy link
Contributor

brillout commented Jul 8, 2022

Yes. I'll update the vite-plugin-ssr CI bug reports as soon as there is a new beta release.

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.

[Vite 3] Legacy Build Bug
3 participants