Skip to content

Address version conflict when used together with firebase-admin #1916

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

Merged
merged 11 commits into from
Jul 1, 2019
Merged

Address version conflict when used together with firebase-admin #1916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7af6ecd
abstract SDK_VERSION
Feiyang1 Jun 25, 2019
9088571
some comment
Feiyang1 Jun 25, 2019
282b916
remove the unnecessary build target
Feiyang1 Jun 25, 2019
1743467
require default
Feiyang1 Jun 25, 2019
5ef2697
[AUTOMATED]: Prettier Code Styling
Feiyang1 Jun 25, 2019
35efa6d
[AUTOMATED]: License Headers
Feiyang1 Jun 25, 2019
da4717c
remove experimental changes
Feiyang1 Jun 25, 2019
092c99c
remove unused var
Feiyang1 Jun 26, 2019
e794be1
only ignore MODULE_NOT_FOUND error
Feiyang1 Jun 26, 2019
c47ea2b
[AUTOMATED]: Prettier Code Styling
Feiyang1 Jun 28, 2019
05f667f
revert module export change
Feiyang1 Jul 1, 2019
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
41 changes: 26 additions & 15 deletions packages/database/index.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
* limitations under the License.
*/

import firebase from '@firebase/app';
import { CONSTANTS, isNodeSdk } from '@firebase/util';
import { FirebaseApp, FirebaseNamespace } from '@firebase/app-types';
import { FirebaseNamespace, FirebaseApp } from '@firebase/app-types';
import { _FirebaseNamespace } from '@firebase/app-types/private';
import { Database } from './src/api/Database';
import { DataSnapshot } from './src/api/DataSnapshot';
Expand All @@ -29,30 +27,26 @@ import * as INTERNAL from './src/api/internal';
import * as TEST_ACCESS from './src/api/test_access';
import './src/nodePatches';
import * as types from '@firebase/database-types';
import { setSDKVersion } from './src/core/version';
import { CONSTANTS, isNodeSdk } from '@firebase/util';

const ServerValue = Database.ServerValue;

/**
* A one off register function which returns a database based on the app and
* passed database URL.
*
* @param app A valid FirebaseApp-like object
* @param url A valid Firebase databaseURL
* @param version custom version e.g. firebase-admin version
*/

const ServerValue = Database.ServerValue;

export function initStandalone(
app: FirebaseApp,
url: string,
version?: string
) {
export function initStandalone(app: FirebaseApp, url: string, version: string) {
/**
* This should allow the firebase-admin package to provide a custom version
* to the backend
*/
CONSTANTS.NODE_ADMIN = true;
if (version) {
firebase.SDK_VERSION = version;
}
setSDKVersion(version);

return {
instance: RepoManager.getInstance().databaseFromApp(app, url),
Expand All @@ -70,6 +64,9 @@ export function initStandalone(
}

export function registerDatabase(instance: FirebaseNamespace) {
// set SDK_VERSION
setSDKVersion(instance.SDK_VERSION);

// Register the Database Service with the 'firebase' namespace.
const namespace = (instance as _FirebaseNamespace).INTERNAL.registerService(
'database',
Expand All @@ -94,7 +91,21 @@ export function registerDatabase(instance: FirebaseNamespace) {
}
}

registerDatabase(firebase);
try {
// If @firebase/app is not present, skip registering database.
// It could happen when this package is used in firebase-admin which doesn't depend on @firebase/app.
// Previously firebase-admin depends on @firebase/app, which causes version conflict on
// @firebase/app when used together with the js sdk. More detail:
// https://github.com/firebase/firebase-js-sdk/issues/1696#issuecomment-501546596
const firebase = require('@firebase/app').default;
registerDatabase(firebase);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this swallow a legit error in registerDatabase()? If the require works fine but there is some error thrown in registerDatabase() itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, updated to only ignore MODULE_NOT_FOUND error

} catch (err) {
// catch and ignore 'MODULE_NOT_FOUND' error in firebase-admin context
// we can safely ignore this error because RTDB in firebase-admin works without @firebase/app
if (err.code !== 'MODULE_NOT_FOUND') {
throw err;
}
}

// Types to export for the admin SDK
export { Database, Query, Reference, enableLogging, ServerValue };
Expand Down
6 changes: 5 additions & 1 deletion packages/database/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/

import firebase from '@firebase/app';
import { FirebaseApp, FirebaseNamespace } from '@firebase/app-types';
import { FirebaseNamespace } from '@firebase/app-types';
import { _FirebaseNamespace } from '@firebase/app-types/private';
import { Database } from './src/api/Database';
import { DataSnapshot } from './src/api/DataSnapshot';
Expand All @@ -28,10 +28,14 @@ import * as INTERNAL from './src/api/internal';
import * as TEST_ACCESS from './src/api/test_access';
import { isNodeSdk } from '@firebase/util';
import * as types from '@firebase/database-types';
import { setSDKVersion } from './src/core/version';

const ServerValue = Database.ServerValue;

export function registerDatabase(instance: FirebaseNamespace) {
// set SDK_VERSION
setSDKVersion(instance.SDK_VERSION);

// Register the Database Service with the 'firebase' namespace.
const namespace = (instance as _FirebaseNamespace).INTERNAL.registerService(
'database',
Expand Down
9 changes: 3 additions & 6 deletions packages/database/src/core/PersistentConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@
* limitations under the License.
*/

import firebase from '@firebase/app';
import { contains, isEmpty, safeGet } from '@firebase/util';
import { contains, isEmpty, safeGet, CONSTANTS } from '@firebase/util';
import { stringify } from '@firebase/util';
import { assert } from '@firebase/util';
import { error, log, logWrapper, warn, ObjectToUniqueKey } from './util/util';
Expand All @@ -25,12 +24,12 @@ import { VisibilityMonitor } from './util/VisibilityMonitor';
import { OnlineMonitor } from './util/OnlineMonitor';
import { isAdmin, isValidFormat } from '@firebase/util';
import { Connection } from '../realtime/Connection';
import { CONSTANTS } from '@firebase/util';
import { isMobileCordova, isReactNative, isNodeSdk } from '@firebase/util';
import { ServerActions } from './ServerActions';
import { AuthTokenProvider } from './AuthTokenProvider';
import { RepoInfo } from './RepoInfo';
import { Query } from '../api/Query';
import { SDK_VERSION } from './version';

const RECONNECT_MIN_DELAY = 1000;
const RECONNECT_MAX_DELAY_DEFAULT = 60 * 5 * 1000; // 5 minutes in milliseconds (Case: 1858)
Expand Down Expand Up @@ -926,9 +925,7 @@ export class PersistentConnection extends ServerActions {
clientName = 'node';
}

stats[
'sdk.' + clientName + '.' + firebase.SDK_VERSION.replace(/\./g, '-')
] = 1;
stats['sdk.' + clientName + '.' + SDK_VERSION.replace(/\./g, '-')] = 1;

if (isMobileCordova()) {
stats['framework.cordova'] = 1;
Expand Down
24 changes: 24 additions & 0 deletions packages/database/src/core/version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* @license
* Copyright 2019 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/** The semver (www.semver.org) version of the SDK. */
export let SDK_VERSION = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this export in combination with setSDKVersion doesn't work, as it exports the current state of the variable:

deps.js:

let foo = 'foo'
function setToBar() {
	foo = 'bar';
}
module.exports = {foo, setToBar};

let deps = require('./deps');
console.log(deps.foo);
deps.setToBar();
console.log(deps.foo);

Prints:

foo
foo

Copy link
Member Author

Choose a reason for hiding this comment

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

It actually works because esm only exports references. The code compiled to cjs format would look like:

exports.foo = 'foo';
function setToBar() {
    exports.foo = 'bar';
}
exports.setToBar = setToBar;


// SDK_VERSION should be set before any database instance is created
export function setSDKVersion(version: string): void {
SDK_VERSION = version;
}
11 changes: 5 additions & 6 deletions packages/database/src/realtime/WebSocketConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ import { RepoInfo } from '../core/RepoInfo';

declare const MozWebSocket: any;

import firebase from '@firebase/app';
import { assert } from '@firebase/util';
import { assert, CONSTANTS as ENV_CONSTANTS } from '@firebase/util';
import { logWrapper, splitStringBySize } from '../core/util/util';
import { StatsManager } from '../core/stats/StatsManager';
import {
Expand All @@ -33,12 +32,12 @@ import {
VERSION_PARAM,
WEBSOCKET
} from './Constants';
import { CONSTANTS as ENV_CONSTANTS } from '@firebase/util';
import { PersistentStorage } from '../core/storage/storage';
import { jsonEval, stringify } from '@firebase/util';
import { isNodeSdk } from '@firebase/util';
import { Transport } from './Transport';
import { StatsCollection } from '../core/stats/StatsCollection';
import { SDK_VERSION } from '../core/version';

const WEBSOCKET_MAX_FRAME_SIZE = 16384;
const WEBSOCKET_KEEPALIVE_INTERVAL = 45000;
Expand Down Expand Up @@ -150,9 +149,9 @@ export class WebSocketConnection implements Transport {
// UA Format: Firebase/<wire_protocol>/<sdk_version>/<platform>/<device>
const options: { [k: string]: object } = {
headers: {
'User-Agent': `Firebase/${PROTOCOL_VERSION}/${
firebase.SDK_VERSION
}/${process.platform}/${device}`
'User-Agent': `Firebase/${PROTOCOL_VERSION}/${SDK_VERSION}/${
process.platform
}/${device}`
}
};

Expand Down