Fix Firestore exports for Node #5532
Conversation
🦋 Changeset detectedLatest commit: c5e701f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Changeset File Check ✅
|
Binary Size ReportAffected SDKs
Test Logs
|
packages/firestore/package.json
Outdated
| "node": "./dist/lite/index.node.cjs.js", | ||
| "node": { | ||
| "require": "./dist/lite/index.node.cjs.js", | ||
| "import": "./dist/lite/index.node.esm2017.js" |
There was a problem hiding this comment.
import needs to point to a file with mjs extension.
| "node": "./dist/index.node.cjs.js", | ||
| "node": { | ||
| "require": "./dist/index.node.cjs.js", | ||
| "import": "./dist/index.node.mjs" |
There was a problem hiding this comment.
Where is the build rule for creating this file?
There was a problem hiding this comment.
It gets the name from whatever is in the main-esm field in package.json
There was a problem hiding this comment.
So here is the build config
There was a problem hiding this comment.
I see. Did you test it in Nodejs - running import {} from @firebase/firestore ?
There was a problem hiding this comment.
I tested it with svelte-kit's SSR(?) build step using firebase/firestore and it works. It does not work when just creating a raw JS file and running it in Node on the command line and it also doesn't work when using @firebase/firestore in either case. The error is odd:
node --experimental-json-modules node.js
(node:78153) ExperimentalWarning: Importing JSON modules is an experimental feature. This feature could change at any time
file:///Users/chholland/repos/sveltekit-firebase/node_modules/@firebase/firestore/dist/index.node.mjs:8
import { version as version$2 } from '@grpc/grpc-js/package.json';
^^^^^^^
SyntaxError: The requested module '@grpc/grpc-js/package.json' does not provide an export named 'version'
I think it's not able to deal with a dependency (grpc-js) still being in cjs format, and svelte-kit uses esbuild so something in the build step is fixing whatever the problem is, unless you import from @firebase/firestore directly in which case it doesn't work. So some special steps are needed to make it work in Node but I'm not sure what they are.
There was a problem hiding this comment.
I've added the workaround in Option 2 here: https://www.stefanjudis.com/snippets/how-to-import-json-files-in-es-modules-node-js/#option-2%3A-leverage-the-commonjs-%60require%60-function-to-load-json-files
It required some changes to tsconfig.json to avoid some TS compiler complaints but it compiles correctly and works in both a Node and a Sveltekit test project. It also works in both projects if I don't change tsconfig and I just tsignore the offending lines (import module from 'module' and const require = module.createRequire(import.meta.url);) so any thoughts on which way is better?
There was a problem hiding this comment.
I'd use ts-ignore with comments here because we don't want these changes to affect browser code which shares the same tsconfig with Node code.
|
LGTM. Does cjs build work correctly ? |
hsubox76
force-pushed
the
ch-firestore-exports
branch
from
b260614
to
f8348bf
Compare
|
Yes, CJS build works in a raw Node 12 project. |
|
@schmidt-sebastian This is the PR that would require dropping Node 8 support (so that we can use |
|
We do have precedent for that. I believe we ran some numbers last time that basically showed that there were no users on Node 6 that used an updated SDK version. I will check tomorrow if I can find some details. |
|
Thank you for this! This looks like a good change to me. I had actually suggested making such a change here: #4846 (comment) I'm the primary maintainer of SvelteKit at the moment and happy to advise on getting SvelteKit better supported if there's anything I can offer insights on. SvelteKit uses Vite which is used by Vue, Astro, and others. SvelteKit leverages the server-side rendering features of Vite much more heavily than other platforms at the moment and so we're the first to hit some of these issues. I expect that as these features are finalized (SSR is in beta) and rolled out to the Vue user base that these issues will start popping up much more regularly since there are many more Vue users than SvelteKit users. Would it be possible make this change to all Firebase packages? E.g. #4846 is this same issue, but reported for |
|
@benmccann Yes, we are planning to do that. |
Oh, should I also get rid of the |
| @@ -2,7 +2,7 @@ | |||
| "name": "@firebase/firestore", | |||
| "version": "3.1.0", | |||
| "engines": { | |||
| "node": "^8.13.0 || >=10.10.0" | |||
| "node": ">=10.10.0" | |||
There was a problem hiding this comment.
Removing Node 8 from this line.
Create
requireandimportsubfields fornodeexports where the latter points tomjsESM builds for Node. Renamemain-esmfields so rollup builds ESM node bundles withmjsextension.Fixes #5499