Make ssr.noExternal shallow

[brillout]

Description

Current behavior in v2: when an SSR dependency some-library is noExternal, then Vite will as well noExternal all some-library's dependencies.

Current behavior in v3 (beta): slightly better because it only noExternal dependencies of some-libraries that cannot be resolved. (Correct me if I'm wrong.)

Suggested solution

Externalize all some-library's dependencies. I.e. only make some-library noExternal.

Priority

Low. (Because AFAICT this doesn't cause the same problems than noExternalize too many SSR user dependencies.)

Alternative

No response

Additional context

No response

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

[patak-dev]

Current behavior in v2: when an SSR dependency some-library is noExternal, then Vite will as well noExternal all some-library's dependencies.

It is the other way around. IIUC, in v2 sub-dependencies of some-library will end up being external

Current behavior in v3 (beta): slightly better because it only noExternal dependencies of some-libraries that cannot be resolved. (Correct me if I'm wrong.)

This is correct, if a sub-dependency is a direct dependency of your project it will be external, but if not they will be noExternal as node will not be able to resolve to them depending on your setup.

What you are proposing here is to keep v2's logic. But as we were discussing in #8983, this is broken if we also don't rewrite imports to resolved node_modules paths that may also break in prod, no?

[brillout]

Since some-library is noExternal, it lives in dist/, which means that, with pnpm (without shamefully-hoist=true), the some-library code living in dist/ cannot resolve its dependencies anymore.

Potential solution: resolve the import statements in dist/: instead of having import 'some-library-dep' we would have import '../../node_modules/.pnpm/[email protected]_biqbaboplfbrettd7655fr4n2y/node_modules/some-library-dep'.

I'm not really sure about rewriting imports for externalized deps since it makes the output code not portable
@bluwy

It means that the user shouldn't modify node_modules after having run $ vite build which I think is a fairly safe assumption. I wonder if there are use cases where dist/ is uploaded without node_modules/. Usually it's either you let the deploy provider both install and build, or you make it on some other machine and upload the whole thing. I'm not aware of any deploy use case where only dist/ is uploaded?

[brillout]

It is the other way around. IIUC, in v2 sub-dependencies of some-library will end up being external

I just tried and it does seem to be as I described. (The VikePress dependency tweemoji is living in dist/.)

this is broken if we also don't rewrite imports to resolved node_modules paths that may also break in prod, no?

Yes exactly, see my previous reply.

[patak-dev]

I think this isn't difficult to implement now that we do lazy SSR externals/noExternal resolving, maybe it could be optional? ssr.shallowNoExternal: true?

[patak-dev]

Or maybe we could use a similar scheme to what @bluwy implemented with

optimizeDeps.include: [ 'dep > nested' ]

And have

ssr.external: [ 'vikepress > tweemoji' ]

And even

ssr.external: [ 'vikepress > *' ]

The global boolean may cause surprises as every other user dep will work as shallow no external too.

[brillout]

Sounds good.

Also maybe: ssr.noExternalShallow. Or if we do it shallow by default (which I think we should): ssr.noExternal => shallow, ssr.noExternalDeep => deep.

(Btw. the naming ssr.noExternal and optimizeDeps was quite confusing to me when I first played with Vite. Maybe we can revisit the naming once the dust settles.)

[patak-dev]

Yes, maybe it should be ssr.bundle? I don't know about making shallow the default, I think I'm missing context to make that call

[brillout]

👍 Also ssr.include if the user wants it to be included in dev as well. And maybe we can say "excluded" instead of "externalized". (I know that Rollup's naming is "external" but I'd suggest we diverge.)

So:

• ssr.bundle = noExternal only in build.
• ssr.include = noExternal for both dev + build.

About the portability issue: since we make shallow/deep configurable, it's not a blocker anymore so we're good.

[bluwy]

Re portability, my concern are for setups that publish dist only and run npm install --prod elsewhere. I think this is still something we should support, as it won't work with this proposal as vikepress and twemoji would be part of devDependencies and installing prod deps would exclude them, which would fail to import in runtime.

This could be fixed though if vikepress is a dependency but it may be wasteful and be in an odd spot IMO. And if it'd already be a dependency, maybe it should stay externalized in the first place 🤔

Perhaps this can be implemented as a Vite plugin for now to test it out.

[brillout]

publish dist only and run npm install --prod elsewhere

I wonder if this has a real world use case; I don't know why one would do this.

Also when using lock files, which is an increasingly ubiquitous practice, this may actually still work.

I think this is still something we should support

Yes I agree and that's why the idea is to support both shallow externalization and deep externalization.

Those who need to build before installation can use deep externalization.

[bluwy]

I wonder if this has a real world use case; I don't know why one would do this.

Also when using lock files, which is an increasingly ubiquitous practice, this may actually still work.

It's the reason why we split dependencies and devDependencies in the first place, so I think it's fairly common. At least on SvelteKit side, this is commonly done for adapter-node. Lock files won't help unless vikepress is a "dependency" which npm install --prod would only then install. If it's a "devDependency", we can't access twemoji even if we resolved to a deep path because it's not installed.

[brillout]

It's the reason why we split dependencies and devDependencies in the first place, so I think it's fairly common.

I see, that makes sense.

Lock files won't help unless vikepress is a "dependency" which npm install --prod would only then install. If it's a "devDependency", we can't access twemoji even if we resolved to a deep path because it's not installed.

Vite only needs to resolve twemoji If it occurs somewhere in the code. If it occurs somewhere in the code then it's not a devDependency to start with.

So I still think a lock file works (as long as the package manager produces a node_modules file structure that is stable given a specific lock file).