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

fix(firestore-bigquery-export) use modules instead of namespaces #1534

Closed
wants to merge 2 commits into from

Conversation

yutak23
Copy link
Contributor

@yutak23 yutak23 commented Apr 6, 2023

This is a PR to correct the error in the following issue.
#1533

After checking where the error is occurring, it appears that there is a problem with the require syntax that is importing the module.
I think that the correct code should be as follows (typescripts build to javascripts).

correct:

const firestore = require("firebase-admin/firestore");

In other words, I think the problem is using the global admin namespace, as in the following code.
https://github.com/firebase/extensions/blob/firestore-bigquery-export-v0.1.31/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/index.ts#L18

This should be redefined as follows (I am aware that it says so in Use modules instead of namespaces).

import { DocumentReference } from 'firebase-admin/firestore';
...
    serializeData(eventData) {
        ...
                if (property.constructor.name === DocumentReference.name) {
                    this.update(property.path);
                }
            }
        });
        return data;
    }

@yutak23
Copy link
Contributor Author

yutak23 commented Apr 6, 2023

This is my first time drafting an issue and creating a PR for OSS.
Cloud you let me know if there is anything I have missed.

@yutak23 yutak23 requested a review from a team as a code owner April 12, 2023 02:08
moduleNameMapper: {
"firebase-admin/firestore":
"<rootDir>/node_modules/firebase-admin/lib/firestore",
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with reference to the following issue.

firebase/firebase-admin-node#1465

Copy link
Contributor Author

@yutak23 yutak23 Apr 12, 2023

Choose a reason for hiding this comment

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

and i confirm npm run test result is ALL Green.

image

Copy link
Contributor

@cabljac cabljac left a comment

Choose a reason for hiding this comment

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

lgtm!

@cabljac
Copy link
Contributor

cabljac commented May 24, 2023

CI isn't running for some reason, closing in favour of #1561

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants