Skip to content

Conversation

nikoferro
Copy link

@nikoferro nikoferro commented Apr 16, 2024

This PR fixes:

  • Web3 import: makes it work after transpiling
  • TSConfig: excludes test from the dist
  • TSConfig: target ES2020
  • Config files: change to JSON format
  • BN.js: included as part of the @metamask/controller-utils updates. Aims to remove BigNumber usage in a future version

@nikoferro nikoferro requested a review from dan437 as a code owner April 16, 2024 12:50
@nikoferro nikoferro requested a review from a team April 16, 2024 12:50
Copy link

socket-security bot commented Apr 16, 2024

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

View full report↗︎

@nikoferro nikoferro changed the title fix: web3 import + bet ter compiler options + dependencies list fix: web3 import + better compiler options + dependencies list Apr 16, 2024
tsconfig.json Outdated
"sourceMap": true,
"strict": true,
"target": "ES2017"
"target": "ESNext"
Copy link
Contributor

@legobeat legobeat Apr 16, 2024

Choose a reason for hiding this comment

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

Why is changing compile target from es2017 to esnext done here? How is it "better"?

Copy link
Author

Choose a reason for hiding this comment

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

@legobeat its to target the latest supported by our TypeScript version. I need to explain better the PR description, but with "better" i was not referring to this change in particular, but the previous configuration was also including test files on the dist, so better in terms of "we are packaging only what we need"

Copy link
Contributor

@legobeat legobeat Apr 17, 2024

Choose a reason for hiding this comment

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

I guess I feel a bit hesitant about this one, as well as the lib removal on :7, timing-wise... The rest looks straightforward enough, but there's also a lot going on here.

Was thinking perhaps the changes in this PR can be broken into more than one PR, so they can be more easily reviewed and merged independently.

But this may be more straightforward to someone more familiar with that context!

Copy link
Author

Choose a reason for hiding this comment

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

in this very particular case, its really straightforward. lib was removed because we dont need definitions for browser environment (since this is only used on mobile), and since our target changed, the ES2020 inclusion in lib, no longer makes sense.

i merged your suggestion to go from ESNext to ES2020 to move slower in terms of target

if its worth mentioning, i have also run integration tests on top of a mobile build using this new set of options and everything works as expected

@nikoferro nikoferro changed the title fix: web3 import + better compiler options + dependencies list fix: web3 import and ts compiler config Apr 17, 2024
@nikoferro nikoferro requested a review from legobeat April 17, 2024 09:55
Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

LGTM!

@nikoferro nikoferro merged commit e7d650b into main Apr 17, 2024
@nikoferro nikoferro deleted the fix/web3 branch April 17, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants