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

Importing getFunctions() throws "self is not defined" during SSR/Static (v9-beta) #4846

Closed
MarcusCemes opened this issue Apr 28, 2021 · 26 comments · Fixed by #5646
Closed

Importing getFunctions() throws "self is not defined" during SSR/Static (v9-beta) #4846

MarcusCemes opened this issue Apr 28, 2021 · 26 comments · Fixed by #5646

Comments

@MarcusCemes
Copy link

MarcusCemes commented Apr 28, 2021

Describe your environment

  • Operating System version: Ubuntu 20.04.2.0
  • Browser version: Chrome 90.0.4430.93
  • Firebase SDK version: 9.0.0-beta.1
  • Firebase Product: functions

Describe the problem

Unable to render a statically built site using Sveltekit. This seems to be a result of Sveltekit's new bundling technique of "using Vite to dump everything into one file before running it through Rollup for tree-shaking and optimisation".

The following line gets included in the bundle when a user's code has import { getFunctions } from "firebase/functions";, even if getFunctions() is not called. During SSR, window, here referenced as self, is not available. This is a import side-effect. fetch() is pollyfilled during SSR by Sveltekit. From what I can tell, it's the ESM build that's being imported.

registerFunctions(fetch.bind(self))

The static build adapter for Sveltekit fails with:

> self is not defined
ReferenceError: self is not defined
    at file:///sandbox/.svelte/output/server/app.js:806:30
    (...)

This is not an attempt to use Firebase during SSR or from Node.js. I would like to include firebase in the browser bundle. A solution is lazily importing getFunctions() from the browser, however, this comes with a latency penalty and an extra network request.

Steps to reproduce:

  1. Add [email protected] to a Sveltekit application
  2. Try to run npm run dev or npm run build (dev will only fail once, as dev-time SSR is not retried a second time)

Edit firebase-self-not-defined

The sandbox is a little weird, it occasionally works, occasionally doesn't. Downloading the sandbox as a ZIP on Windows and Ubuntu still causes the build error.

Relevant Code:

Offending line

registerFunctions(fetch.bind(self));

Failure case
import { getFunctions } from "firebase/functions";

export function functions() {
    return getFunctions(app());
}
@google-oss-bot
Copy link
Contributor

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

@MarcusCemes
Copy link
Author

MarcusCemes commented Apr 29, 2021

After more testing, even lazy-loading getFunctions() via import("firebase/functions") from a central file does not solve the issue. The lazy import

// $lib/functions.ts
const { cloudFunction } = await import("$lib/glacier/core/functions");

is being transformed into

// .svelte/output/app.js
const {cloudFunction: cloudFunction2} = await Promise.resolve().then(function() {
    return functions;
  });

This may be for performance reasons (lazy-loads are unnecessary during SSR) or for tree-shaking bundling purposes (Rollup). I can try opening up an issue on the SvelteKit repository, but this side-effect behaviour may cause issues with other SSR/SSG frameworks in the future. SvelteKit's advice is to run any window-dependent code after a browser check, but this is not possible in this case, making firebase functions unusable with SvelteKit.

@MarcusCemes MarcusCemes changed the title Importing getFunctions() throws "self is not defined" during SSR/Static (Firebase Beta) Importing getFunctions() throws "self is not defined" during SSR/Static (v9-beta) Apr 29, 2021
@Feiyang1 Feiyang1 added the v9 label Apr 30, 2021
@Feiyang1
Copy link
Member

Feiyang1 commented Apr 30, 2021

Just want to clarify that the issue happens during static site generation which does happen in Nodejs? so the error makes sense.

And you're right that it's caused by the side effect of the module, so it happens even if you don't call getFunctions(). I'd like to get rid of the side effect eventually, but we need to do some research first which I haven't had a chance to do.

As a workaround for now, is it possible to configure svelte (rollup) to pick up the node build instead of the browser build of firebase/functions? To do that, you need to tell rollup to use the main field in package.json.

@MarcusCemes
Copy link
Author

You are correct, the issue does indeed happen during static site generation in a Node.js environment. I understand that firebase expects a browser environment, but SSR requires running some code on the server, before being sent to the browser.

I would argue that this behaviour does not make sense and is therefore undesirable. No function from the firebase module is ever used. Import-time initialisation is a bit of a hack from the CommonJS days, introduces start-up delays and makes it impossible to intercept errors. In my mind, there is no technical barrier that would keep initialisation from happening upon the first invocation of a function. I'm not sure how other SSR frameworks (Next.js, Nuxt, ...) have mitigated this issue so far, perhaps Webpack's module system has some form of lazy execute-once-required policy.

While it's true that SvelteKit and Vite are relatively "new kids on the block" that do things differently to battle-tested CommonJS-based frameworks, I commend them on their bravery to try and give the JS community that push into the ESM world that brings it more in line with traditional programming languages. I would like to add that fetch is available in the global scope during SvelteKit SSR (library implementation, now in most frameworks), that is not the problem, Node.js just doesn't have a window (or self) object but instead a global (and this respectively, incidentally also works in Chrome).

I'm not sure if such a workaround would work, my understanding is that Vite will attempt to transform CJS into ESM, likely resulting in the same issue. Having said that, I have just discovered a SvelteKit-specific workaround. Moving the firebase dependency to devDependencies will no longer inline firebase/functions code in the server bundle, but rather use the ES import { ... } from ... syntax. From what I call tell, Node.js does not execute any top-level code (the side effects) when resolving the import, a sort of "runtime tree-shaking", neither does the browser, and the SDK still works.

In ESM, the module loader runs in asynchronous phases. In the first phase, it parses the script to detect calls to import and export without running the imported script. (Source)

Heads up, I'm not sure how browsers handle this case, but if ESM hits more mainstream frameworks, it may become a problem. For now, my workaround is adequate, although I would like the new firebase SDK to embrace true ESM modularity.

@benmccann
Copy link

As a workaround for now, is it possible to configure svelte (rollup) to pick up the node build instead of the browser build of firebase/functions?

If I'm looking at the right package source (I'm on my phone and may not be), then I see the following:

"main": "dist/index.node.cjs.js",
"browser": "dist/index.esm.js",
"module": "dist/index.esm.js",

This seems like it may not be setup correctly. module is something used by Node.js and thus implies server side. If index.esm.js is isomorphic that's fine, but if it's intended to be used only on the browser I think that's wrong and the field should either be removed from package.json or an ESM version of the server side build can be bundled and listed there

@Feiyang1
Copy link
Member

@benmccann Interesting! We have been using the module field for browser builds all the time. I just checked rollup documentation again, but didn't see any text that says whether it should be used for nodejs build or browser build. so my understanding is that there is no consensus on that, and it's up to the lib authors. Do you have any link that says module field should point to nodejs build?

@benmccann
Copy link

module isn't quite standardized and is more of a de facto standard, so isn't covered in the official npm docs. However, main has always been the Node entry point with browser being the client-side entry point. These are the two standardized fields explicitly covered in the documentation.. The point of module is to differentiate ESM entry point from CommonJS entry point as discussed in the Node docs and Rollup docs. In both these places module is explicitly being discussed as an alternative to main, which is the server-side entry point (with the client entry point being browser).

@benmccann
Copy link

benmccann commented Jul 28, 2021

Node only knows about main. Webpack, Rollup, etc. use module. By default, they both consider module for both server and client, but only consider browser for client. So I do think that module should be able to run on the server and that browser should be used for the client.

@benmccann
Copy link

benmccann commented Jul 28, 2021

It looks like the codesandbox example is using an outdated version of SvelteKit and Vite. If I update it (updated code on Github), then Vite's behavior does match that described in the docs and it does use the CJS version on the server-side. However, that code path shouldn't be executed in Vite and there's another bug causing it to go down that path: vitejs/vite#4425

We should still leave this open though as I still think it's wrong to put a browser-only build in module and Firebase should still be fixed as well

@MarcusCemes
Copy link
Author

I've re-created the sandbox using a new Sveltekit skeleton project (so removed the explicit vite dependency), updated all the dependencies to their respective latest versions (beta/next) and locked in the versions to maintain reproducibility. The bug is still there, you just need to download as a ZIP and run it locally, I think it's a caching issue with codesandbox, Ctrl+F5 usually helps to trigger the error.

I have managed to get this to work on my own project through extensive dynamic import(...) and if (browser) checks, but it would be preferable to be able to import the cloud functions module without side-effects and only instantiate it once it is loaded (once it is needed) in the browser, without additional code-splitting head-aches.

Edit firebase-self-not-defined (2)

Terminal error
ReferenceError: self is not defined
    at eval (/node_modules/@firebase/functions/dist/index.esm2017.js?v=ecda2b99:651:30)
    at instantiateModule (C:\Users\Marcus\dev\experimental\sandbox\node_modules\vite\dist\node\chunks\dep-c1a9de64.js:73464:166)

@rawkode
Copy link

rawkode commented Aug 9, 2021

@MarcusCemes I'm hitting this error and I'd love to work around it as you have. Would you be able to share some code to show how to protect against the self is not defined error, please?

@rawkode
Copy link

rawkode commented Aug 9, 2021

Found something that works, for anyone else that runs into this:

<script lang="ts">
	import { browser } from '$app/env';

	const doIt = () => {
		if (browser) {
			import('firebase/functions').then((fns) => {
				const functions = fns.getFunctions();

                                // ... your code here
			});
		} else {
			console.log('Not in browser');
		}
	};
</script>

@MarcusCemes
Copy link
Author

MarcusCemes commented Aug 11, 2021

Workaround

Updated on 2021-08-12

After a little experimentation, the only problematic firebase module I can find is firebase/functions, which is easy to workaround. Firestore and the other modules should work fine using the normal ES import syntax.

Simply make sure that you never import anything from firebase/functions directly using the ES import syntax. Instead, load the module using the dynamic import(...) syntax. Vite will put that library in a separate bundle (with the offending browser-only line) that you can load-on-demand at runtime from the browser, just make sure to call the function after a DOM event (such as a click) or in onMount, otherwise, it might get loaded during SSR.

Your implementation should work fine, here's my solution that makes it a little easier and avoids promises and imports everywhere. The downside of the dynamic import is that the firebase/functions module will be added to the vendors chunk and automatically loaded on every page, there's some discussion about this over here.

// $lib/functions.ts
import type { HttpsCallable } from "firebase/functions"; // type imports are fine, they are removed during transpilation

export function cloudFunction<P, R>(name: string): HttpsCallable<P, R> {
  return async function (params) {
    const { getFunctions, httpsCallable } = await import("firebase/functions");
    const service = getFunctions(firebase(), "europe-west3");
    return httpsCallable<P, R>(service, name)(params);
  };
}
<!-- index.svelte -->
<script lang="ts">
  import { cloudFunction } from "$lib/functions";

  const fn = cloudFunction("someFunction");

  async function onClick() {
    await fn({ time: Date.now() });
  }
</script>

<button on:click={onClick}>Execute function</button>

@Xstyler85
Copy link

I have the same problem:

ReferenceError: self is not defined
    at eval (/node_modules/@firebase/functions/dist/index.esm2017.js?v=093f313f:651:30)
    ...

I'm looking forward to an easier workaround / fix.

@Feiyang1
Copy link
Member

@MarcusCemes IIUC, Vite translates import 'firebase/functions' to require('firebase/functions') during SSR, which should pick up the node version of functions correctly. Is it not the case?

@MarcusCemes
Copy link
Author

@Feiyang1 I am not too familiar with the inner workings of the Vite/Rollup/Sveltekit trio, but inspecting the generated server/app.js file for SSR, the ESM version of firebase/functions seems to have been inlined into the file, like so, and not converted into CommonJS require(...) imports:

const name = "@firebase/functions-exp";
const version = "0.0.900-exp.8b4d7550f";
registerFunctions(fetch.bind(self));      // offending line
registerVersion(name, version);
const Test = create_ssr_component(($$result, $$props, $$bindings, slots) => {

As stated in my above comment, moving firebase to devDependencies does create an explicit import ... from in server/app.js for some reason instead of inlining, this does solve the issue, but only because the Node.js ESM loader does not execute any top-level side-effect code! The offending lines never run, and the behaviour may be the same in the browser using the ESM bundle, I'm not sure.

The firebase/functions's package.json advertises its bundles like so:

{
  "name": "firebase-exp/functions",
  "main": "dist/index.cjs",
  "browser": "dist/index.esm.js",
  "module": "dist/index.esm.js",
  "typings": "dist/functions/index.d.ts",
  "type": "module"
}

My guess is that Vite is inlining the module bundle (which happens to be the browser bundle as well) as it is an ESM first bundler. Technically, ESM should work in a Node.js environment, it's purely an import mechanism and does not require any browser-specific APIs.

@benmccann
Copy link

@Feiyang1 if you'd like to continue using the module field for browser builds all the time, the other way to fix this would be to add exports to the package.json to clarify which builds should be used by Node vs the browser

@Bandit
Copy link

Bandit commented Sep 23, 2021

FWIW I encounter this (and this sveltejs/kit#612) issue when switching from import { ... } from 'firebase/firestore' to import {...} from 'firebase/firestore/lite'

@MarcusCemes
Copy link
Author

To follow up on the discussion, here's my experience with SvelteKit/Vite/Firebase, for anyone else who might find this insightful.

There's a lot going on with the Firebase SDK that Vite and Node.js can't deal with (without resorting to the Admin SDK). I've encountered all sorts of errors after accidentally including a Firebase import somewhere in the dependency chain, whether on purpose or by accident with VSCode's Intellisense. I have a spaghetti of browser checks and lazy imports, and it's still not working correctly.

I tried moving SSR-related Firestore code into a Cloud Function that I can query during SSR without the SDK, before realising that Timestamps and DocumentReferences are not trivially serialized over JSON (and it plays havoc with types...). Convenient, but locked-in.

That, and also this (5-10 seconds for me...), and this, has led me to start the move away from Firebase. I was very excited at first, but it's quite difficult to integrate these stacks together at the moment, without too many unnecessary abstractions. I've no doubt that for some it may be a fantastic solution.

The client SDK is used by a lot more than just SvelteKit/Vite users, it was designed with CommonJS and Webpack in mind, I'm guessing that messing with the package.json could break things for someone else, even if it is more correct. I'm sure the Google engineers are doing their best, and I have the utmost respect for their work, but for my use case, it was simply too much hassle to be able to get anything done, I miss the days of a simple JSON API and concise hand-written code.

@Feiyang1
Copy link
Member

So I made a prerelease with exports support for all firebase packages including functions, but it doesn't seem to solve the problem using @MarcusCemes's repro.

@benmccann Do you mind giving it a try, using [email protected]?

The package.json of @firebase/functions has the exports field defined as:

  "exports": {
    "node": {
      "import": "./dist/esm-node/index.node.esm.js",
      "require": "./dist/index.node.cjs.js"
    },
    "browser": "./dist/index.esm2017.js",
    "module": "./dist/index.esm2017.js",
    "default": "./dist/index.esm2017.js"
  }

I expect Vite to use ./dist/esm-node/index.node.esm.js pointed by the import field for SSR, however it still seems to load ./dist/index.esm2017.js, and I got the same error as before:

[vite] Error when evaluating SSR module /node_modules/@firebase/functions/dist/index.esm2017.js?v=1b5ebfec:
ReferenceError: self is not defined
    at eval (/node_modules/@firebase/functions/dist/index.esm2017.js?v=1b5ebfec:674:30)
    at instantiateModule (/Users/feiyangc/Projects/repros/firebase-self-not-defined/node_modules/vite/dist/node/chunks/dep-994e0558.js:69063:166)

@benmccann
Copy link

Hi @Feiyang1, thank you for taking a look at this. Your change did allow me to get past that error!

I did get a two other errors, but I would say to go ahead and ship your change. If you send a PR I'd be happy to lend it a set of eyes. One suggestion I would make is to also export package.json as some developer tooling tries to read package.json and will not be able to with the code snippet you pasted above

The most serious remaining issue is a Vite bug. I will work with the Vite team to fix it, but in the meantime it looks like it can be worked around by setting external: ['whatwg-url', 'node-fetch'] in the Vite config.

However, there was one more error originating from Firebase. Vite was complaining vociferously about this Firebase sourcemap, which appears to be empty:

[email protected]/node_modules/firebase/functions/dist$ cat index.mjs.map 
{"version":3,"file":"index.mjs","sources":[],"sourcesContent":[],"names":[],"mappings":""}

As a result, I get the error message:

TypeError: Line must be greater than or equal to 1, got -1

It would be nice if that could be fixed as well, but it could possibly be tracked in a new issue.

@bluwy
Copy link

bluwy commented Oct 29, 2021

I expect Vite to use ./dist/esm-node/index.node.esm.js pointed by the import field for SSR, however it still seems to load ./dist/index.esm2017.js, and I got the same error as before:

The reason why it happens is more related to how node loads modules. If the package is of "type": "commonjs", all .js files are treated as CJS code, even if it's written in ESM. So when in matches the exports, no file matches the desired extension and it loads the one in default always.

So a fix for this is to make sure ESM files always end with .mjs so that node loads properly.

@benmccann
Copy link

It should be noted though that I'm not getting that error. I think that @MarcusCemes test repo was outdated. Here's a working version: https://github.com/benmccann/sveltekit-firebase. (Note that it won't give the other errors I mentioned because I added the ssr.external workaround to svelte.config.js, so you need to remove that line if you want to see the sourcemap error)

@Feiyang1
Copy link
Member

There is a package.json file in ./dist/esm-node with {"type": "module"}, so we don't need the mjs extension.

@bluwy
Copy link

bluwy commented Oct 29, 2021

Ah I see. My bad, that's really strange then.

@Feiyang1
Copy link
Member

@benmccann Nice! Glad it works.

The sourcemap is generated by rollup and I think it's working as intended. The source code only contains one line

export * from '@firebase/functions';

, so it makes sense that the sourcemap is empty? Maybe it's something vite can also look into?

Good point of adding package.json to the exports, though I think the node team is considering to make it a default behavior without needing lib authors to do it explicitly. I will do it for now as a workaround.

Here is the PR - #5646
Would appreciate your review! Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants