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

v1.7.2 #688

Closed
wants to merge 13 commits into from
Closed

v1.7.2 #688

wants to merge 13 commits into from

Conversation

pedrouid
Copy link
Contributor

@pedrouid pedrouid commented Jan 5, 2022

  • replace bn.js with BigInt
  • replace query-string with URLSearchParams

TimDaub and others added 7 commits December 12, 2021 14:33
In the @walletconnect/[email protected], query-string accounts for 4.3% of
the overall bundle size. However, client-side support for
URLSearchParams has reached mainstream and so we can just use it instead
of including more bundle-size-increasing dependencies.

- Fixes #667
@vercel
Copy link

vercel bot commented Jan 5, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-app – ./examples/react-app

🔍 Inspect: https://vercel.com/walletconnect/react-app/4Q7FhELaWG5wdcw4E3gYQAFHUxmV
✅ Preview: Failed

[Deployment for a2c16f6 failed]

react-wallet – ./examples/react-wallet

🔍 Inspect: https://vercel.com/walletconnect/react-wallet/GWYbm7ppAzjWjdcKv7Jxo3Vtvyo3
✅ Preview: Failed

[Deployment for a2c16f6 failed]

canary-react-app – ./examples/react-app

🔍 Inspect: https://vercel.com/walletconnect/canary-react-app/6bW5qEsXJ9NN5hxNgCpSYVhDZht7
✅ Preview: Failed

[Deployment for a2c16f6 failed]

canary-react-wallet – ./examples/react-wallet

🔍 Inspect: https://vercel.com/walletconnect/canary-react-wallet/45Bp1DecHjHZrEhesXn6kKxsi6f8
✅ Preview: Failed

[Deployment for a2c16f6 failed]

@pedrouid
Copy link
Contributor Author

@TimDaub you can see below a comparison between v1.7.1 and v1.7.2-rc.0 which includes both PRs you have submitted and we can see substantial bundle reduction 💪 Thanks a lot for your contributions!

Only thing missing now is testing agaisnt react-native applications to support as low as v0.60 potentially

Screenshot 2022-01-11 at 12 10 06

@TimDaub
Copy link

TimDaub commented Jan 11, 2022

wow this is super cool! thanks for sharing @pedrouid

@xzilja
Copy link
Contributor

xzilja commented Feb 28, 2022

@pedrouid Tested these cases with react native running on hermes engine

  console.log(parseInt('1010', 10))
  console.log(parseInt('0x1F7', 16))
  console.log(BigInt(9007199254740991).toString())
  console.log(BigInt(9007199254740991).toString(16))
  console.log(new URLSearchParams('q=URLUtils.searchParams&topic=api').toString())

BigInt - is not supported at all, related issue facebook/hermes#510
parseInt - Is fine as expected
URLSearchParams - Doesn't error, but it returns empty result to me, where as I expected q=URLUtils.searchParams&topic=api (which happens in browser)

So overall this whole change would be breaking rn

@pedrouid
Copy link
Contributor Author

That's a shame! We will need to develop separate target libraries to support these changes for NodeJS and Browser while still using the current dependencies for React-Native

@bkrem bkrem closed this Mar 2, 2023
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.

4 participants