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.

Sign up for GitHub

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

Jump to bottom

Fix Firestore exports for Node #5532

Merged
merged 9 commits into from
Oct 19, 2021
Merged

Fix Firestore exports for Node #5532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
78a2bbe
Fix firestore exports field
hsubox76 Sep 22, 2021
a8107d8
add changeset
hsubox76 Sep 22, 2021
67400fc
[EMPTY] trigger ci
hsubox76 Sep 23, 2021
7a48599
correct lite mjs import path
hsubox76 Sep 28, 2021
d2b37a2
Add createRequire
hsubox76 Sep 29, 2021
5ab9103
Revert "Add createRequire"
hsubox76 Sep 29, 2021
f8348bf
Add tsignores
hsubox76 Oct 5, 2021
5e57f25
Change to minor bump.
hsubox76 Oct 5, 2021
c5e701f
Remove Node 8 from package.json
hsubox76 Oct 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/big-cooks-drum.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@firebase/firestore': minor
---

Fix exports field to also point to Node ESM builds. This change requires Node.js version 10+.
2 changes: 1 addition & 1 deletion packages/firestore/lite/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"name": "@firebase/firestore-lite",
"description": "A lite version of the Firestore SDK",
"main": "../dist/lite/index.node.cjs.js",
"main-esm": "../dist/lite/index.node.esm2017.js",
"main-esm": "../dist/lite/index.node.mjs",
"module": "../dist/lite/index.browser.esm2017.js",
"browser": "../dist/lite/index.browser.esm2017.js",
"react-native": "../dist/lite/index.rn.esm2017.js",
Expand Down
14 changes: 10 additions & 4 deletions packages/firestore/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"name": "@firebase/firestore",
"version": "3.1.0",
"engines": {
"node": "^8.13.0 || >=10.10.0"
"node": ">=10.10.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing Node 8 from this line.

},
"description": "The Cloud Firestore component of the Firebase JS SDK.",
"author": "Firebase <firebase-support@google.com> (https://firebase.google.com/)",
Expand Down Expand Up @@ -49,16 +49,22 @@
},
"exports": {
".": {
"node": "./dist/index.node.cjs.js",
"node": {
"require": "./dist/index.node.cjs.js",
"import": "./dist/index.node.mjs"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the build rule for creating this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It gets the name from whatever is in the main-esm field in package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here is the build config

firebase-js-sdk/packages/firestore/rollup.config.js

Line 84 in 2ad6ce8

file: pkg['main-esm'],

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Did you test it in Nodejs - running import {} from @firebase/firestore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
"default": "./dist/index.esm2017.js"
},
"./lite": {
"node": "./dist/lite/index.node.cjs.js",
"node": {
"require": "./dist/lite/index.node.cjs.js",
"import": "./dist/lite/index.node.mjs"
},
"default": "./dist/lite/index.browser.esm2017.js"
}
},
"main": "dist/index.node.cjs.js",
"main-esm": "dist/index.node.cjs.esm2017.js",
"main-esm": "dist/index.node.mjs",
"react-native": "dist/index.rn.js",
"browser": "dist/index.esm2017.js",
"module": "dist/index.esm2017.js",
Expand Down
11 changes: 10 additions & 1 deletion packages/firestore/src/platform/node/grpc_connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,16 @@
* limitations under the License.
*/

// This is a hack fix for Node ES modules to use `require`.
// @ts-ignore To avoid using `allowSyntheticDefaultImports` flag.
import module from 'module';

import {
Metadata,
GrpcObject,
credentials as GrpcCredentials,
ServiceError
} from '@grpc/grpc-js';
import { version as grpcVersion } from '@grpc/grpc-js/package.json';

import { Token } from '../../api/credentials';
import { DatabaseInfo } from '../../core/database_info';
Expand All @@ -35,6 +38,12 @@ import { logError, logDebug, logWarn } from '../../util/log';
import { NodeCallback, nodePromise } from '../../util/node_api';
import { Deferred } from '../../util/promise';

// This is a hack fix for Node ES modules to use `require`.
// @ts-ignore To avoid using `--module es2020` flag.
const require = module.createRequire(import.meta.url);
// eslint-disable-next-line @typescript-eslint/no-require-imports
const { version: grpcVersion } = require('@grpc/grpc-js/package.json');

const LOG_TAG = 'Connection';
const X_GOOG_API_CLIENT_VALUE = `gl-node/${process.versions.node} fire/${SDK_VERSION} grpc/${grpcVersion}`;

Expand Down