8000 Fcm GAPIC type conversion by maceonthompson · Pull Request #1354 · firebase/firebase-admin-node · GitHub
[go: up one dir, main page]
Skip to content

Fcm GAPIC type conversion #1354

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

Jump to bottom
Merged
merged 36 commits into from
Aug 9, 2021
Merged

Fcm GAPIC type conversion #1354

merged 36 commits into from
Aug 9, 2021

Conversation

maceonthompson
Copy link

Add private method that converts from public messaging.Message type to the similar but generated IMessage type.

@maceonthompson maceonthompson requested a review from hiranya911 July 2, 2021 18:15
@maceonthompson maceonthompson self-assigned this Jul 2, 2021
hiranya911
hiranya911 suggested changes Jul 2, 2021
View reviewed changes
Copy link
Contributor
@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Few minor changes suggested. Also let's work on getting the unit tests to pass.

@hiranya911
Copy link
Contributor

Also sync up/rebase with the base branch so the gulpfile changes are included in the revision history. Without it we cannot run integration tests on this branch directly.

I was able to run integration tests by manually merging the gulpfile changes to this branch on my local machine:

% npm run test:integration -- --grep messaging

> firebase-admin@9.9.0 test:integration /Users/hkj/Projects/firebase-admin-node/public
> mocha test/integration/*.ts --slow 5000 --timeout 20000 --require ts-node/register "--grep" "messaging"

  admin.messaging
    ✓ send(message, dryRun) returns a message ID

Nice work 👍

@maceonthompson
Import generated types & convert Message -> generated Message
a81cdc2
@maceonthompson
Add JSDoc to convertToClientMessage method
3bc7f20
@maceonthompson
Fix error where private Message type was used as object
91f00f2
@maceonthompson
Add support for different public Message types
66b6a18
@maceonthompson
Add getAppKey() as a precursor to using the generated client
69173a4
@maceonthompson
Switch to using generated client in send()
01ddff8
@maceonthompson
Remove TODOS
f76848b
@maceonthompson
Temporary adjustment to generated client to avoid errors at runtime.
4acfe87
@maceonthompson
Merge branch 'maceo-fcm-gapic-type-conversion' of github.com:firebase…
f0225a9 
…/firebase-admin-node into maceo-fcm-gapic-type-conversion
@maceonthompson
Convert send non error expects to use sinon stub
7d1dfa4
@maceonthompson
refactor Sinon behavior to be more in line with original code
73da589
@maceonthompson
Add createFirebaseErrorFromGapicError
43b9cde 
Converts returned GAPIC errors to `FirebaseError`s
@maceonthompson
Add error handling for send() with the generated client
4cdb520
@maceonthompson
Simplify createFirebaseErrorFromGapicError
146bfa1
@maceonthompson
Address casting comment on PR in messaging.ts
134c621
@maceonthompson
Fix unit tests to accomodate new generated client.
cc435ab 
Note: currently commented out majority of `send` error test cases
@maceonthompson
Use the same client instance for multiple calls
4bd6059 
instead of creating a new service client each `send()` call
hiranya911
hiranya911 suggested changes Jul 23, 2021
View reviewed changes
Copy link
Contributor
@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Please address the open comments from this and past reviews, and we should be good to go.

hiranya911
hiranya911 reviewed Jul 23, 2021
View reviewed changes
hiranya911
hiranya911 suggested changes Aug 2, 2021
View reviewed changes
Copy link
Contributor
@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes and for the responses to earlier questions. Looks pretty good. I only pointed out a few nits, but this is almost done.

@maceonthompson maceonthompson added release:stage Stage a release candidate and removed release:stage Stage a release candidate labels Aug 4, 2021
hiranya911
hiranya911 approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor
@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM 👍

src/messaging/messaging.ts
}

options.projectId = projectId;
}).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop this line safely, and it would still work.

@maceonthompson maceonthompson merged commit 3900ea9 into maceo-fcm-gapic-client Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hiranya911 hiranya911 hiranya911 approved these changes

@lahirumaramba lahirumaramba Awaiting requested review from lahirumaramba

Assignees

@maceonthompson maceonthompson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maceonthompson @hiranya911 @lahirumaramba
0