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

Overly high JS baseline requirement, from using URL.canParse() #78

Open
Tristan971 opened this issue Jul 21, 2024 · 5 comments · May be fixed by #80
Open

Overly high JS baseline requirement, from using URL.canParse() #78

Tristan971 opened this issue Jul 21, 2024 · 5 comments · May be fixed by #80
Labels

Comments

@Tristan971
Copy link

Tristan971 commented Jul 21, 2024

Hello,

In here, the library has begun using URL.canParse():

https://github.com/braintree/sanitize-url/blame/cdd33eb72e9f56eff7b3c49f1c5ed133838b9956/src/index.ts#L24

This imposes a surprisingly strict requirement on browser versions (https://caniuse.com/?search=canParse):

  • Safari/iOS 17.0
  • Firefox 125
  • Chromium 120

As far as I can tell, it has as a result only been widely available since ~ December 2023.

I don't mind it necessarily, but it is a huge enough jump that I'd suggest making it in a major version update.

I'm not sure what the previous minimal browser version set was exactly, but at least up to iOS 14 worked, going by the browsers we had successfully tested on our own website.

@akphi
Copy link

akphi commented Jul 22, 2024

though rather unrelated, I would like to mention that this makes testing with Jest a little less convenient since URL.canParse is not implemented in jsdom

@erquhart
Copy link

erquhart commented Aug 6, 2024

This also breaks compatibility with React Native. I realize this was to address a security issue, but it's also a breaking change in a minor release, and should be reverted for that reason alone.

Related PR: #77

cc/ @ibooker

@ibooker
Copy link
Contributor

ibooker commented Aug 6, 2024

Hello,
@Tristan971 Thanks for reporting this issue with URL.canParse(). We'll take a closer look at this issue. (Internal Tracking: BTWEB-171)

@erquhart We do not specifically support compatibility with React Native. If you can provides some details about what no longer works, we can better understand if its something we can address.

@ibooker ibooker added the triaged label Aug 6, 2024
@erquhart
Copy link

erquhart commented Aug 6, 2024

@ibooker I don't see anything in the repo stating explicit support for any platform, browser etc., I'm just adding React Native to the list of environments for which this library no longer works.

I don't have a separate issue to outline, just that the use of URL.canParse() is not supported in React Native, as well as a fair amount of browser versions and jsdom, as mentioned by others.

aloisklink added a commit to aloisklink/sanitize-url that referenced this issue Sep 23, 2024
[URL.canParse][1] was only pretty recently created, and is not
available in many environments.

We can instead wrap a `URL` creation in a `try..catch`, as listed in
MDN. Unfortunately, this is a bit slower, but at least it works almost
everywhere.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/URL/canParse_static

Fixes: braintree#78
aloisklink added a commit to aloisklink/sanitize-url that referenced this issue Sep 23, 2024
This would have caught
like braintree#78, as
`URL.canParse()` was only added in Node.JS v18.17
@aloisklink aloisklink linked a pull request Sep 23, 2024 that will close this issue
@chrisvanpatten
Copy link

Agree, 7.1.0 should have been released as 8.0.0; effectively dropping support for older Nodes, whether intentional or not, is a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants