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

[web3.js] Eliminate dependency on URL class #27349

Merged
merged 3 commits into from
Aug 24, 2022
Merged

[web3.js] Eliminate dependency on URL class #27349

merged 3 commits into from
Aug 24, 2022

Conversation

steveluscher
Copy link
Contributor

@steveluscher steveluscher commented Aug 23, 2022

Problem

The URL class is not available in all runtimes, namely React Native. For this, web3.js currently depends on a polyfill.

Thing is, our use of URL is very constrained:

  • Determine if a URL is valid
  • Determine if a URL starts with https:
  • Parse the port number out of a URL and modify it

I'm all for relying on built-ins, but when the built-in doesn't exist on a target platform, it's worth taking a look at whether you can get away with a minimal implementation of it rather than a full polyfill.

image

Summary of Changes

  • Eliminate the dependency on the URL class in favor of purpose-built userspace implementations of URL parsing.
  • Remove React Native URL polyfill and regenerate lockfiles

Test plan

  • Search for new URL in src/; no more uses.

Addresses solana-labs/solana-web3.js#1103
Closes: #27001

@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #27349 (e6df162) into master (e779032) will decrease coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   solana-labs/solana#27349     +/-   ##
=========================================
- Coverage    76.9%    76.8%   -0.2%     
=========================================
  Files          48       47      -1     
  Lines        2505     2507      +2     
  Branches      355      359      +4     
=========================================
- Hits         1927     1926      -1     
- Misses        448      452      +4     
+ Partials      130      129      -1     

@steveluscher steveluscher merged commit 5975176 into solana-labs:master Aug 24, 2022
@steveluscher steveluscher deleted the why-url-why branch August 24, 2022 18:02
@jordaaash
Copy link
Contributor

Sorry for the late review. Looks good, it's nice to kill that polyfill.

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.

Web3.js react-native-url-polyfill dependency causing issues on react web apps
2 participants