SSR fails on sub-package imports when exports field is defined (can't SSR Firebase v9)

[benmccann]

Describe the bug

tryNodeResolve fails on Firebase.

It swallows this exception from _resolveExports inside resolveExports:

Missing "." export in "firebase" package

It seems reasonable that Firebase doesn't export this because they don't allow you to import firebase. You have to import a sub-package like firebase/functions, etc. I don't see anything in the Node docs that says you must export .

As a result, Firebase is not treated as external eventhough it provides a CJS build. This in turn causes SSR to fail when using Firebase

Reproduction

https://github.com/benmccann/sveltekit-firebase

System Info

Vite 2.4.4

Used Package Manager

npm

Logs

Vite doesn't output any logs. I sent a PR to fix that: #4426

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/vue-next instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.

[brillout]

@benmccann

What do you think of following proposal?

1. By default, Vite always treats SSR dependencies as external.
2. Vite doesn't try to resolve externalized SSR dependencies. For example if the user code has an import such as import 'firebase/functions', then Vite transpiles it to require('firebase/functions') and leaves the require as-is to be consumed by Node.js. This means that from Vite's perspective the firebase dependency is only a bunch of import statements that Vite transpiles to require statements. Vite doesn't need nor try to resolve these import statements.
3. When bundling server-code for production (e.g. for Cloudflare Workers), Vite uses the Node.js built-in require.resolve() instead of using a custom resolver.

This would solve the Firebase problem as Vite wouldn't even touch the Firebase library in dev.

[benmccann]

What if a package is only distributed as ESM? I think we'd still need to detect CJS vs ESM because sometimes we'll need to do an import and sometimes we'll need to do a require. But I agree that it would be safest to do the minimal possible when there's an external CJS module. (#4340)

I think the particular case in this issue is tricky because Vite tries to handle this import as a single package, but with the exports field there's multiple entry points and Vite isn't really setup to be able to handle that

[brillout]

What if a package is only distributed as ESM? I think we'd still need to detect CJS vs ESM because sometimes we'll need to do an import and sometimes we'll need to do a require.

When the user-side code's package.json has type: module then yes Vite should leave the user-side code's import statements as-is instead of transpiling them to require.

Bottom line: even with ESM support, there is no need to resolve SSR dependencies.

From an SSR perspective, what lives in node_modules/ is none of Vite's problem and Vite should let Node.js handle it. Only for the browser-side and for server-side bundling does Vite need to dig into node_modules/.

I think the particular case in this issue is tricky because Vite tries to handle this import as a single package, but with the exports field there's multiple entry points and Vite isn't really setup to be able to handle that

I see.

Just to be sure we are on the same page: do you as well see my proposal as a solution to the Firebase problem?

[benmccann]

I'm going to preface this by saying that there are so many combinations of this stuff that I always get a bit confused

I still wonder with your proposal how would you treat a third-party ESM module? As an example, see here: vitejs/vite-plugin-react#30. Vite today can't externalize unified because it is ESM-only with "type": "module". If you add it to ssr.external it tries to require it and fails. We should probably detect this and try to import it instead. But that would require detecting whether it's ESM or CJS.

Just to be sure we are on the same page: do you as well see my proposal as a solution to the Firebase problem?

I don't know if I totally understand the details of the proposal well enough yet, but right now I'm not seeing how it'd fix the issue. Your proposal still seems helpful regardless (I just chimed in with an explanation for that on #4340 (comment)). But for this issue, the problem is resolving the module and it's not clear to me how you skip that since you need to figure out whether to do an import or require. Perhaps you could distribute Vite as type: "module" and then always import stuff found in node_modules?

[brillout]

@benmccann

See my comment about ESM #4340 (comment).

Let me know whether you now see #4340 as the solution to the Firebase problem.

[benmccann]

Unfortunately, it won't fix Firebase because loading is much more broken than resolving. If you fix the resolving then you'll get ReferenceError: exports is not defined (#2579). And it's not just Firebase. It's every single library built with Rollup (possibly Webpack and other bundlers too. I haven't checked them)

[brillout]

Why isn't Firebase externalized?

[brillout]

Shouldn't Firebase be externalized anyways?

[benmccann]

It should be externalized, but isn't because of the bug discussed in this issue. See the issue description for an explanation.

However, unfortunately externalizing it doesn't help because loading is broken. Vite tries to do new Function instead of require and then fails. We should just have Node do the loading as well

[brillout]

Part of my proposal is to always externalize SSR dependencies (unless it's added in ssr.noExternalize).

Because Firebase is externalized, the loading will be done by Node.js, not by Vite.

With my ESM Plan proposal, because all SvelteKit users have type: "module" in their package.json, Vite would leave the import statements as-is, instead of transpiling them to require. (That's the whole point of setting type: "module" after all: using ESM instead of CJS.)

It will then be Node.js that 1. resolves the firebase import and 2. loads the firebase code. Things will then work. Why am I so confident? Because Firebase is already used by bunch of vanilla Node.js users today. Although type: "module" may break things, but that would be a Firebase problem not supporting ESM users, and certainly not a Vite problem since Vite stays out of the picture here.

The whole point of my proposal is to get Vite out of the way and let Node.js handle things.

Does that make more sense?

[benmccann]

Because Firebase is externalized, the loading will be done by Node.js, not by Vite.

Just to be clear, are you saying that changing it to do the loading would also be part of the plan? If so, I'd highly support that.

If you're saying that's already supposed to be done then I think there might be another issue to solve because as far as I can tell, Node never loads libraries today even if I add them to ssr.external. This line is almost always hit in my experience and fails quite frequently:

vite/packages/vite/src/node/ssr/ssrModuleLoader.ts

Line 127 in 887c247

new Function(

[brillout]

If so, I'd highly support that.

Yes and I'm glad we agree 👌:-).

If you're saying that's already supposed to be done

Are you sure it's not already the case? Isn't that precisely the whole point of externalizing a dep? To me "externalize" = let Node.js handle it.

Either way, externalized deps should definitely not be loaded by Vite (IMO).

I will sum up my proposal into an actionable step-by-step plan.

[benmccann]

Are you sure it's not already the case? Isn't that precisely the whole point of externalizing a dep? To me "externalize" = let Node.js handle it.

Actually, I'm not so sure now. It looks like it often does work. I will have to see if I can come up with a good reproduction. Unfortunately most of the cases I'm seeing also are hitting another bug and it's difficult to untangle

[aleclarson]

This is likely fixed by #5210. Could someone verify?

[benmccann]

There were just some new instructions for testing changes added to the contributing guide (79c2a49). I tried to follow these steps, but was unable to

@aleclarson when I try to build your PR (#5210), I get some build failures:

> vite@2.6.3 build-temp-types
> tsc --emitDeclarationOnly --outDir temp/node -p src/node

../../node_modules/@types/body-parser/index.d.ts:14:10 - error TS2305: Module '"connect"' has no exported member 'NextHandleFunction'.

14 import { NextHandleFunction } from 'connect';
            ~~~~~~~~~~~~~~~~~~

../../node_modules/@types/cookies/index.d.ts:161:40 - error TS2503: Cannot find namespace 'connect'.

161     connect(keys: string[] | Keygrip): connect.NextHandleFunction;
                                           ~~~~~~~

../../node_modules/@types/express-serve-static-core/index.d.ts:504:18 - error TS2430: Interface 'Response<ResBody, StatusCode>' incorrectly extends interface 'ServerResponse'.
  Types of property 'req' are incompatible.
    Type 'Request<ParamsDictionary, any, any, ParsedQs> | undefined' is not assignable to type 'IncomingMessage'.
      Type 'undefined' is not assignable to type 'IncomingMessage'.

504 export interface Response<ResBody = any, StatusCode extends number = number> extends http.ServerResponse, Express.Response {
                     ~~~~~~~~

../../node_modules/@types/express/index.d.ts:59:55 - error TS2344: Type 'Response<any>' does not satisfy the constraint 'ServerResponse'.
  Types of property 'req' are incompatible.
    Type 'Request<ParamsDictionary, any, any, ParsedQs> | undefined' is not assignable to type 'IncomingMessage'.
      Type 'undefined' is not assignable to type 'IncomingMessage'.

59     var static: serveStatic.RequestHandlerConstructor<Response>;

Perhaps if you rebase against main it will fix these errors?

[benmccann]

Hmm, I'm not sure why I had difficulty building the PR before, but I just tried again and was able to build it. I can confirm it does indeed fix this issue!

[benmccann]

This is fixed in 2.7.0-beta.3: #5544

[worxto]

This is fixed in 2.7.0-beta.3: #5544

Still catch this @2.7.0-beta.3

[Niputi]

@worxto please open issue with a reproduction