-
Notifications
You must be signed in to change notification settings - Fork 900
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
Conversation
// @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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 = ''; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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;
@schmidt-sebastian gentle ping :) |
There was a problem hiding this 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
3ba7f0c
to
c47ea2b
Compare
This PR will allow firebase-admin to remove
@firebase/app
from its dependency list.@firebase/app
being a dependency infirebase-admin
causes version conflict whenfirebase-admin
andfirebase
are used together. More detail: #1696 (comment)The PR includes the following changes
version.ts
in@firebase/database
, so the source code doesn't import@firebase/app
directly for SDK_VERSION.SDK_VERSION
inversion.ts
in the entry files (index.ts and index.node.ts), so we can setSDK_VERSION
differently depending on the target.import firebase from '@firebase/app'
torequire('@firebase/app').default
inindex.node.ts
for handlingfirebase-admin
use case. It allows@firebase/database
to work infirebase-admin
without@firebase/app
The corresponding PR in
firebase-admin
: firebase/firebase-admin-node#586