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

Hardcode grpc-js version #5771

Merged
merged 3 commits into from
Dec 8, 2021
Merged

Hardcode grpc-js version #5771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc7db2b
Hardcode grpc version
hsubox76 Nov 30, 2021
ff265d5
Restore import.meta
hsubox76 Nov 30, 2021
303c512
update comment
hsubox76 Dec 1, 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
17 changes: 10 additions & 7 deletions packages/firestore/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,20 @@
* limitations under the License.
*/

import tmp from 'tmp';
import json from '@rollup/plugin-json';
import { version as grpcVersion } from '@grpc/grpc-js/package.json';
import alias from '@rollup/plugin-alias';
import typescriptPlugin from 'rollup-plugin-typescript2';
import typescript from 'typescript';
import replace from 'rollup-plugin-replace';
import json from '@rollup/plugin-json';
import copy from 'rollup-plugin-copy';
import replace from 'rollup-plugin-replace';
import { terser } from 'rollup-plugin-terser';
import typescriptPlugin from 'rollup-plugin-typescript2';
import tmp from 'tmp';
import typescript from 'typescript';

import pkg from './package.json';
import { generateBuildTargetReplaceConfig } from '../../scripts/build/rollup_replace_build_target';

import pkg from './package.json';

const util = require('./rollup.shared');

// Customize how import.meta.url is polyfilled in cjs nodejs build. We use it to be able to use require() in esm.
Expand Down Expand Up @@ -69,7 +71,8 @@ const nodePlugins = function () {
]
}),
replace({
'process.env.FIRESTORE_PROTO_ROOT': JSON.stringify('src/protos')
'process.env.FIRESTORE_PROTO_ROOT': JSON.stringify('src/protos'),
'__GRPC_VERSION__': grpcVersion
})
];
};
Expand Down
12 changes: 3 additions & 9 deletions 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,10 +15,6 @@
* 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,
Expand All @@ -38,11 +34,9 @@ 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 requireInESM = module.createRequire(import.meta.url);
// eslint-disable-next-line @typescript-eslint/no-require-imports
const { version: grpcVersion } = requireInESM('@grpc/grpc-js/package.json');
// TODO: Fetch runtime version from grpc-js/package.json instead
Copy link

Choose a reason for hiding this comment

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

IIRC in order to be able to merge this PR, we are waiting on changes from Firestore to surface the GRPC that is why this is not complete? or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is a stopgap measure so that users can use the SDK without errors for now. In the future if and when the grpc-js team (outside of Firebase) is able to provide us with an export, the plan is we'll revert this and use that instead.

Copy link

Choose a reason for hiding this comment

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

I see so we are not actually intending on passing any sort of version at the moment, but rather a placeholder string and in some future we will pass along the version. Makes sense, just wasn't clear if this TODO was for now or later, thank you for clarifying

Copy link
Contributor Author

@hsubox76 hsubox76 Dec 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh no, we will pass a version. The version will be grabbed by Rollup at build time based on the version of grpc-js installed when we build the Firebase SDK. Ideally we want the version installed by the user in their node_modules dir, which may differ, but since we specify the version in @firebase/firestore's package.json dependencies, that should be the version they have for the most part.

Line 75 in rollup.config.js above does the string replacement.

// when there's a cleaner way to dynamic require JSON in both Node ESM and CJS
const grpcVersion = '__GRPC_VERSION__';

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