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

URL incorrectly adds trailing slash #24428

Closed
leethree opened this issue Apr 12, 2019 · 13 comments
Closed

URL incorrectly adds trailing slash #24428

leethree opened this issue Apr 12, 2019 · 13 comments
Labels
Bug Stale There has been a lack of activity on this issue and it may be closed soon.

Comments

@leethree
Copy link

leethree commented Apr 12, 2019

🐛 Bug Report

URL polyfill in React Native 0.59 is broken because it's not compliant to WHATWG URL Standard. And it's broken even for the simplest use cases.

To Reproduce

const url = new URL('https://facebook.github.io/react-native/img/header_logo.png');
console.log(url);

Expected Behavior

https://facebook.github.io/react-native/img/header_logo.png

Actual Behavior

https://facebook.github.io/react-native/img/header_logo.png/

Note the trailing slash causes the URL points to a 404 page.

Environment

  React Native Environment Info:
    System:
      OS: macOS 10.14.4
      CPU: (12) x64 Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
      Memory: 823.64 MB / 16.00 GB
      Shell: 3.2.57 - /bin/bash
    Binaries:
      Node: 10.15.0
      Yarn: 1.15.2
      npm: 6.5.0
      Watchman: 4.9.0
    SDKs:
      iOS SDK:
        Platforms: iOS 12.2, macOS 10.14, tvOS 12.2, watchOS 5.2
      Android SDK:
        API Levels: 28
        Build Tools: 28.0.3
        System Images: android-28 | Intel x86 Atom_64, android-28 | Google APIs Intel x86 Atom
    IDEs:
      Android Studio: 3.3 AI-182.5107.16.33.5314842
      Xcode: 10.2/10E125 - /usr/bin/xcodebuild
    npmPackages:
      react: 16.8.3 => 16.8.3 
      react-native: 0.59.4 => 0.59.4 

Possible Cause

this._url = url;
if (!this._url.endsWith('/')) {
this._url += '/';
}

This code was introduced in #22901 .

@leethree
Copy link
Author

PR #22901 also caused issue #23922

@matthargett
Copy link
Contributor

Hi @leethree ! Thanks for taking the time to report an issue and give a reproduction case.

I'll submit a PR that fixes this issue along with a test in the next day or two.

With regard to #23922 , the URLSearchParams wasn't even more barebones and the polyfill wouldn't interact with React Native's custom URL that supports blobs in a performant way. Before #22901, using something like apollo-server would crap out much earlier with a different error.

@stale
Copy link

stale bot commented Aug 2, 2019

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Aug 2, 2019
@leethree
Copy link
Author

leethree commented Aug 2, 2019

hey bot, the issue is not resolved. leave us alone

@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Aug 2, 2019
@redreceipt
Copy link
Contributor

redreceipt commented Oct 28, 2019

I'll second this issue, having to add a goofy workaround

    const url = new URL(baseURL);
    // NOTE: RN adds a trailing slash
    // https://github.com/facebook/react-native/issues/24428
    url._url = url.toString().endsWith('/')
      ? url.toString().slice(0, -1)
      : url.toString();

@charpeni
Copy link
Contributor

👋 You should check this out: https://github.com/charpeni/react-native-url-polyfill.

@charpeni charpeni reopened this Nov 25, 2019
@stale
Copy link

stale bot commented Feb 23, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 23, 2020
@redreceipt
Copy link
Contributor

Still here

@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 23, 2020
@stale
Copy link

stale bot commented May 23, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label May 23, 2020
@leethree
Copy link
Author

Hi bot, I don't think it's fixed.

@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label May 27, 2020
@charpeni
Copy link
Contributor

This issue will probably never be fixed, and if it does, it will be with a goofy workaround which is probably adding more issues behind it. See: #25719 (comment).

In a near future, I plan to deprecated RN's URL in favor of https://github.com/charpeni/react-native-url-polyfill, so I would suggest doing the jump right away.

@stale
Copy link

stale bot commented Aug 29, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Aug 29, 2020
@stale
Copy link

stale bot commented Sep 5, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
6 participants