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

build: Remove duplicate @ethersproject/bignumber npm pkg and update to ^5.7.0 #21054

Merged

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Sep 26, 2023

Description

This PR removes a duplicate @ethersproject/bignumber npm package. keeps the one in dependencies and removes the other one from devDependencies.

This PR also updates the package from ^5.6.2 → ^5.7.0. I did not find any reasons to not do this.

Screenshot 2023-09-27 at 12 56 33 AM

Manual testing steps

build app and check app is working

Fixes #21055

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained:
    • What problem this PR is solving.
    • How this problem was solved.
    • How reviewers can test my changes.
  • I’ve indicated what issue this PR is linked to: Fixes #???
  • I’ve included tests if applicable.
  • I’ve documented any added code.
  • I’ve applied the right labels on the PR (see labeling guidelines).
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@github-actions
Copy link
Contributor

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.

@digiwand digiwand added dependencies Pull requests that update a dependency file team-confirmations-secure-ux-PR PRs from the confirmations team labels Sep 26, 2023
@digiwand digiwand changed the title build: Remove duplicate @ethersproject/bignumber npm package build: Remove duplicate @ethersproject/bignumber npm package and update to ^5.7.0 Sep 26, 2023
@digiwand digiwand changed the title build: Remove duplicate @ethersproject/bignumber npm package and update to ^5.7.0 build: Remove duplicate @ethersproject/bignumber npm pkg and update to ^5.7.0 Sep 26, 2023
@digiwand digiwand marked this pull request as ready for review September 26, 2023 23:34
@digiwand digiwand requested a review from a team as a code owner September 26, 2023 23:34
@digiwand digiwand requested a review from adonesky1 September 26, 2023 23:34
@legobeat
Copy link
Contributor

Makes sense to not have the same package present in both dependencies and devDependencies 👍

Could we also bump the remaining dependencies entry in package.json to ^5.7.0?

@metamaskbot
Copy link
Collaborator

Builds ready [847300e]
Page Load Metrics (839 ± 394 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint86128104115
domContentLoaded6811693136
load801896839822394
domInteractive6811693136
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@abdelrahmansheta16
Copy link

Hello @digiwand, can you explain how you have added labels to your PR, as this is my first time contributing to a github issue.

@digiwand
Copy link
Contributor Author

Hello @abdelrahmansheta16, of course! on the browser version, there should be a "Labels" section on the right-hand side. clicking on it should allow you to search and add/remove labels. Mobile version is found in the "ⓘ" icon button.

you might have noticed a "Check PR has required labels / check-pr-labels" requirement check. This contains multiple checks including checking the label "external-contributor" (I believe this is the one you'll want) or the label prepended with "team-" exists.

in case you are interested in learning more about specific gh action checks, they can be found in the scripts folder. e.g. .github/scripts/check-pr-has-required-labels.ts.

thanks for asking! happy coding, and if you have any other questions, feel free to reach out again

@digiwand
Copy link
Contributor Author

@legobeat ah, forgot to push up the changes. Updated to ^5.7.0. Thanks!

@abdelrahmansheta16
Copy link

image
@digiwand appreciate your response but as you can see in the image above there isn't any buttons near the "Label" section.

@digiwand
Copy link
Contributor Author

@abdelrahmansheta16 this one
pr

@abdelrahmansheta16
Copy link

@digiwand its unclickable

@digiwand
Copy link
Contributor Author

@abdelrahmansheta16 o.0 odd. hm. I've added labels to your PR for now. Sorry to hear you're running into this. Not seeing a permission setting for this

There are no buttons but clicking on the label row with "Labels" should open the dropdown

@@ -381,7 +381,6 @@
"@babel/preset-react": "^7.0.0",
"@babel/preset-typescript": "^7.16.7",
"@babel/register": "^7.5.5",
"@ethersproject/bignumber": "^5.7.0",
"@lavamoat/allow-scripts": "^2.3.1",
"@lavamoat/lavapack": "^5.2.4",
"@metamask/auto-changelog": "^2.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think creating a comment thread for this will allow the conversation to continue after I merge

@abdelrahmansheta16 does your cursor change to a pointer hand when you hover over the row?

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (8a54f65) 68.35% compared to head (9aa40d3) 68.35%.
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #21054   +/-   ##
========================================
  Coverage    68.35%   68.35%           
========================================
  Files         1007     1007           
  Lines        40269    40269           
  Branches     10773    10773           
========================================
  Hits         27524    27524           
  Misses       12745    12745           

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

@metamaskbot
Copy link
Collaborator

Builds ready [9aa40d3]
Page Load Metrics (1021 ± 409 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint861781142512
domContentLoaded751721042412
load8822811021852409
domInteractive751721042412
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@digiwand digiwand merged commit 8e220e9 into develop Sep 27, 2023
9 checks passed
@digiwand digiwand deleted the build-rm--duplicate-dep-and-update-@ethersproject/bignumber branch September 27, 2023 12:01
@github-actions github-actions bot locked and limited conversation to collaborators Sep 27, 2023
@metamaskbot metamaskbot added the release-11.3.0 Issue or pull request that will be included in release 11.3.0 label Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Pull requests that update a dependency file release-11.3.0 Issue or pull request that will be included in release 11.3.0 team-confirmations-secure-ux-PR PRs from the confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove duplicate @ethersproject/bignumber npm package
5 participants