Skip to content

refactor(notifications): use contentful package as dev dependency#26381

Merged
Prithpal-Sooriya merged 1 commit intodevelopfrom
nofify/refactor-move-contentful-package-to-dev-dep
Aug 13, 2024
Merged

refactor(notifications): use contentful package as dev dependency#26381
Prithpal-Sooriya merged 1 commit intodevelopfrom
nofify/refactor-move-contentful-package-to-dev-dep

Conversation

@Prithpal-Sooriya
Copy link
Copy Markdown
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Aug 13, 2024 •
edited
Loading

Description

We only use this package for its types, so can be treated as a dev dependency. This will also resolve the audit issue currently seen in CI.

Open in GitHub Codespaces

Related issues

Fixes: CI issues, and better reflects the use case of this dependency.

Manual testing steps

Re-test notifications flow

  1. Onboard with new or existing wallet
  2. Enable notifications
  3. Perform a swap on Polygon (a chain we support on-chain notifications for)
  4. See if notifications appear.

For Notification Devs: As there is no feature announcements yet, manually change the code to point to a portfolio feature announcement. I've tested and it seems that we still correctly show a feature announcement to users.

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@Prithpal-Sooriya
refactor: move contentful package to dev dependency
2606849 
we only use this package for its types, so can be treated as a dev dependency
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 13, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review August 13, 2024 09:19
@Prithpal-Sooriya Prithpal-Sooriya requested a review from a team as a code owner August 13, 2024 09:19
@socket-security
Copy link
Copy Markdown

socket-security bot commented Aug 13, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/qs@6.11.0 None 0 229 kB ljharb

🚮 Removed packages: npm/qs@6.11.2

View full report↗︎

@metamaskbot metamaskbot added the team-notifications-deprecated DEPRECATED: please use "team-assets" instead label Aug 13, 2024
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Aug 13, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Prithpal-Sooriya
Prithpal-Sooriya commented Aug 13, 2024
View reviewed changes
package.json
"chalk": "^4.1.2",
"chokidar": "^3.6.0",
"concurrently": "^8.2.2",
"contentful": "^10.8.7",
Copy link
Copy Markdown
Contributor Author

@Prithpal-Sooriya Prithpal-Sooriya Aug 13, 2024

Choose a reason for hiding this comment

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

I have moved this from "dependencies" to "dev-dependencies". We only use this package for its types, so we don't strictly need this in our builds.

Copy link
Copy Markdown
Contributor Author

@Prithpal-Sooriya Prithpal-Sooriya Aug 13, 2024

Choose a reason for hiding this comment

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

As this is now a dev dependency, it also resolves the yarn audit issues surrounding axios.

matteoscurati
matteoscurati approved these changes Aug 13, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@matteoscurati matteoscurati left a comment

Choose a reason for hiding this comment

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

LGTM

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 13, 2024

Builds ready [2606849]
Page Load Metrics (284 ± 255 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6312396157
domContentLoaded85223126
load421577284531255
domInteractive85223126
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.12%. Comparing base (1fef871) to head (2606849).

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop   #26381   +/-   ##
========================================
  Coverage    70.12%   70.12%           
========================================
  Files         1434     1434           
  Lines        50285    50285           
  Branches     13890    13890           
========================================
  Hits         35259    35259           
  Misses       15026    15026

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Prithpal-Sooriya Prithpal-Sooriya merged commit cad49eb into develop Aug 13, 2024
@Prithpal-Sooriya Prithpal-Sooriya deleted the nofify/refactor-move-contentful-package-to-dev-dep branch August 13, 2024 10:15
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2024
@metamaskbot metamaskbot added the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 13, 2024
@metamaskbot metamaskbot added release-12.2.0 Issue or pull request that will be included in release 12.2.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Aug 28, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 28, 2024

Missing release label release-12.2.0 on PR. Adding release label release-12.2.0 on PR and removing other release labels(release-12.4.0), as PR was cherry-picked in branch 12.2.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mathieuartu mathieuartu mathieuartu approved these changes

+1 more reviewer

@matteoscurati matteoscurati matteoscurati approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-12.2.0 Issue or pull request that will be included in release 12.2.0 team-notifications-deprecated DEPRECATED: please use "team-assets" instead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Prithpal-Sooriya @metamaskbot @matteoscurati @mathieuartu