-
Notifications
You must be signed in to change notification settings - Fork 985
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.
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
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 :) |
schmidt-sebastian
left a comment
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/appfrom its dependency list.@firebase/appbeing a dependency infirebase-admincauses version conflict whenfirebase-adminandfirebaseare used together. More detail: #1696 (comment)The PR includes the following changes
version.tsin@firebase/database, so the source code doesn't import@firebase/appdirectly for SDK_VERSION.SDK_VERSIONinversion.tsin the entry files (index.ts and index.node.ts), so we can setSDK_VERSIONdifferently depending on the target.import firebase from '@firebase/app'torequire('@firebase/app').defaultinindex.node.tsfor handlingfirebase-adminuse case. It allows@firebase/databaseto work infirebase-adminwithout@firebase/appThe corresponding PR in
firebase-admin: firebase/firebase-admin-node#586