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

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

Merged
merged 11 commits into from
Jul 1, 2019

Conversation

Feiyang1
Copy link
Member

@Feiyang1 Feiyang1 commented Jun 25, 2019

This PR will allow firebase-admin to remove @firebase/app from its dependency list. @firebase/app being a dependency in firebase-admin causes version conflict when firebase-admin and firebase are used together. More detail: #1696 (comment)

The PR includes the following changes

  1. Introduce a new file version.ts in @firebase/database, so the source code doesn't import @firebase/app directly for SDK_VERSION.
  2. set SDK_VERSION in version.ts in the entry files (index.ts and index.node.ts), so we can set SDK_VERSION differently depending on the target.
  3. change the static import firebase from '@firebase/app' to require('@firebase/app').default in index.node.ts for handling firebase-admin use case. It allows @firebase/database to work in firebase-admin without @firebase/app

The corresponding PR in firebase-admin: firebase/firebase-admin-node#586

// @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

*/

/** 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;

@Feiyang1
Copy link
Member Author

@schmidt-sebastian gentle ping :)

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

LGMT based on your ESM research

@Feiyang1 Feiyang1 merged commit c24ecf3 into master Jul 1, 2019
@Feiyang1 Feiyang1 deleted the fei-admin-compat branch July 1, 2019 17:35
@firebase firebase locked and limited conversation to collaborators Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants