feat: Remove GCM #450
feat: Remove GCM #450
Conversation
# Conflicts: # .gitignore # package-lock.json # package.json # spec/GCM.spec.js # spec/ParsePushAdapter.spec.js # src/GCM.js # src/ParsePushAdapter.js # src/index.js
|
I will reformat the title to use the proper commit message syntax. |
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughGCM implementation and tests were removed; Android push now uses FCM and requires a firebaseServiceAccount. package.json dependency deleted, ParsePushAdapter updated (new APIs and stricter Android config), exports now expose FCM instead of GCM, and tests adjusted to FCM. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Adapter as ParsePushAdapter
participant FCM as FCM Sender
participant APNS as APNS Sender
Note over Adapter: classifyInstallations + validate config
Client->>Adapter: send(data, installations)
Adapter->>Adapter: classifyInstallations(installations)
alt Android installs present
Adapter->>Adapter: validate firebaseServiceAccount
alt valid
Adapter->>FCM: send(androidPayload, androidTokens)
FCM-->>Adapter: results
else missing/invalid
Adapter-->>Client: error(PUSH_MISCONFIGURED)
end
end
opt iOS installs present
Adapter->>APNS: send(apnsPayload, deviceTokens)
APNS-->>Adapter: results
end
Adapter->>Adapter: aggregate results (use [...promises].flat(2))
Adapter-->>Client: aggregated results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.js (1)
3-3: Update the outdated comment.The comment still references GCM, but this PR removes GCM in favor of FCM for Android push notifications.
Apply this diff to update the comment:
-// PushAdapter, it uses GCM for android push, APNS for ios push. +// PushAdapter, it uses FCM for android push, APNS for ios push.
🧹 Nitpick comments (3)
spec/ParsePushAdapter.spec.js (1)
498-499: Formatting changes look good.The spy setup formatting changes improve code consistency.
Also applies to: 545-545
src/ParsePushAdapter.js (2)
41-46: LGTM! Web and Expo configuration unchanged.The Web and Expo push sender initialization remains functionally unchanged, with only formatting improvements.
75-75: LGTM! Formatting improvements for consistency.The formatting changes improve code readability and consistency.
Also applies to: 82-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.json(0 hunks)spec/GCM.spec.js(0 hunks)spec/ParsePushAdapter.spec.js(14 hunks)src/GCM.js(0 hunks)src/ParsePushAdapter.js(3 hunks)src/index.js(1 hunks)
💤 Files with no reviewable changes (3)
- src/GCM.js
- spec/GCM.spec.js
- package.json
🧰 Additional context used
🧬 Code graph analysis (3)
src/index.js (1)
src/ParsePushAdapter.js (1)
ParsePushAdapter(12-96)
src/ParsePushAdapter.js (5)
src/FCM.js (3)
pushType(160-160)FCM(20-48)LOG_PREFIX(9-9)src/APNS.js (2)
APNS(8-319)LOG_PREFIX(6-6)src/WEB.js (2)
WEB(9-104)LOG_PREFIX(7-7)src/EXPO.js (2)
EXPO(25-105)LOG_PREFIX(7-7)src/PushAdapterUtils.js (1)
devices(20-20)
spec/ParsePushAdapter.spec.js (1)
src/FCM.js (1)
FCM(20-48)
🔇 Additional comments (11)
src/index.js (1)
17-17: LGTM! FCM replaces GCM in public API.The import and export changes correctly replace GCM with FCM, aligning with the broader migration in this PR.
Also applies to: 22-22
spec/ParsePushAdapter.spec.js (6)
4-4: LGTM! Import and assertion updated for FCM.The import statement and type assertion correctly reflect the migration from GCM to FCM.
Also applies to: 23-23
40-41: LGTM! Android configuration and expectations updated for FCM.The test correctly configures Android push with
firebaseServiceAccountand validates that the sender is an FCM instance.Also applies to: 67-67
77-93: LGTM! Test validates FCM initialization for both platforms.The test correctly verifies that both Android and iOS can be configured to use FCM when
firebaseServiceAccountis provided.
95-122: LGTM! Test validates FCM for Android with APNS for iOS.The test correctly verifies the hybrid scenario where Android uses FCM and iOS uses APNS.
124-140: LGTM! Test validates FCM for both Apple and Android platforms.The test correctly verifies that both Apple and Android platforms can use FCM when configured with
firebaseServiceAccount.
403-404: LGTM! Android configuration updated to use FCM.The test configurations correctly specify
firebaseServiceAccountfor Android push with FCM.Also applies to: 597-598
src/ParsePushAdapter.js (4)
32-40: LGTM! Apple platforms support FCM or APNS.The logic correctly routes Apple platforms (iOS/tvos/osx) to FCM when
firebaseServiceAccountis present, otherwise defaults to APNS.
60-62: New public method exposes valid push types.The
getValidPushTypes()method provides external access to the internalvalidPushTypesarray. This is a useful addition for consumers who need to validate push types.
64-66: New static method exposes installation classification.The
classifyInstallations()static method delegates to the utility function, making it accessible to external consumers. This is a breaking change if the method signature or behavior differs from previous versions.Verify that exposing this method as a public static API is intentional and won't cause breaking changes for existing consumers. Based on the PR summary indicating this is a feature PR, this appears to be a deliberate API enhancement.
91-94: Verify flattening depth for all sender response formats. Ensure each adapter’ssend()method (APNS, FCM, WEB, EXPO) resolves to arrays nested no deeper than two levels; adjust the.flat()depth if any return structures exceed that.
…-adapter into moumouls/remove-gcm # Conflicts: # package-lock.json # package.json # spec/ParsePushAdapter.spec.js # src/ParsePushAdapter.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #450 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 9 8 -1
Lines 1351 1172 -179
==========================================
- Hits 1351 1172 -179 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/ParsePushAdapter.js (1)
53-55: Update error message to reflect FCM requirement (not GCM).Message is outdated given GCM removal. Clarify that a Firebase service account is required for Android/FCM.
Apply this diff:
- throw new Parse.Error(Parse.Error.PUSH_MISCONFIGURED, - 'GCM Configuration is invalid'); + throw new Parse.Error( + Parse.Error.PUSH_MISCONFIGURED, + 'Firebase service account is required for Android/FCM push notifications' + );
🧹 Nitpick comments (2)
src/ParsePushAdapter.js (2)
76-84: Avoid logging/returning full payload in error; tighten message.Embedding
${data}in logs/responses can leak payload content and usually prints “[object Object]”. Use a concise, non-sensitive message and correct “Can not” → “Cannot”.Apply this diff:
- if (Array.isArray(devices) && devices.length > 0) { + if (Array.isArray(devices) && devices.length > 0) { if (!sender) { - log.verbose(LOG_PREFIX, `Can not find sender for push type ${pushType}, ${data}`) + log.verbose(LOG_PREFIX, `Cannot find sender for push type ${pushType}`) const results = devices.map((device) => { return Promise.resolve({ device, transmitted: false, - response: { 'error': `Can not find sender for push type ${pushType}, ${data}` } + response: { error: `Cannot find sender for push type ${pushType}` } }) }); sendPromises.push(Promise.all(results));
61-63: Return a copy to prevent external mutation.
getValidPushTypes()exposes the internal array by reference. Return a shallow copy.Apply this diff:
- getValidPushTypes() { - return this.validPushTypes; - } + getValidPushTypes() { + return [...this.validPushTypes]; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/FCM.spec.js(10 hunks)src/ParsePushAdapter.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/ParsePushAdapter.js (1)
src/PushAdapterUtils.js (1)
devices(20-20)
spec/FCM.spec.js (1)
src/FCM.js (1)
FCM(20-51)
🪛 GitHub Actions: ci
spec/FCM.spec.js
[error] 1-1: Cannot find package '@jasmine/core' imported from spec/FCM.spec.js (ERR_MODULE_NOT_FOUND). Command failed: npm run test (TESTING=1 c8 ./node_modules/.bin/jasmine).
🔇 Additional comments (2)
spec/FCM.spec.js (2)
31-33: LGTM on assertion style updates.Using
toThrow()on invalid constructor args reads fine.
75-76: LGTM on spyOn formatting changes.No behavioral change; safe refactor.
Also applies to: 102-103, 130-131, 157-158, 184-186, 213-215, 229-231, 248-250
|
Removal of GCM is a breaking change. Could you please do this is a dedicated PR, separate from other changes. |
|
I think we should just merge this PR and release the new major version, since GCM has been shut down for six years, and this library doesn’t receive or need many updates. The GCM removal, library update, and audit fixes resolve several critical dependency issues. @mtrezza Currently, using this library (which is a dependency of Parse Server) raises critical issues in Dependabot and forces developers to fork it or use systems like yarn patch or npm patch Does it means we need to release also a major version of parse-server once parse-server is updated ? since parse-server will not support GCM also ? |
|
Removing an API is always a breaking change. We cannot anticipate the behavior in all possible scenarios. For example, GCM is EOL so API calls may be no-ops, but removing the API may cause a crash. Other example, someone may relay to another GCM compatible 3rd party service instead of sending to the Google API. There are also other reasons why we make changes in individual PRs, each one creating their own alpha release, see the contribution guide. Yes, that will also be a breaking change for Parse Server. |
…-adapter into moumouls/remove-gcm # Conflicts: # package-lock.json # package.json
|
i applied request changes @mtrezza , there is some changes in package-lock because i ran npm audit fix |
3 participants
New Pull Request Checklist
Issue Description
Related issue: #427
Approach
Remove GCM and run npm audit fix to patch deps
TODOs before merging
Summary by CodeRabbit
New Features
Breaking Changes
Chores
Tests