Make `ssr.noExternal` shallow

brillout commented 2 years ago

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.)

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

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

patak-dev commented 2 years ago

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 commented 2 years ago

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/some-library-dep@0.0.23_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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

👍 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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).

vitejs / vite