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

homepage in package.json breaks react-create-app if it contains a fragment identifier (#foo) #8514

Conversation

michaelmcmillan
Copy link

@michaelmcmillan michaelmcmillan commented Feb 19, 2020

This is a subtle bug which causes create-react-app to strip a character from the homepage URL and then crash. It was discovered by @cyrille-arundo, @geevb and myself.

Steps to reproduce

  1. npx create-react-app my-app
  2. Add a URL with a fragment identifier to package.json's homepage: https://domain.com/page#hello"
  3. Run npm start
  4. Watch it blow up in all its glory.
Compiled successfully!

You can now view my-app in the browser.

  Local:            http://localhost:3000/pag
  On Your Network:  http://10.10.10.86:3000/pag

Note that the development build is not optimized.
To create a production build, use yarn build.

URIError: Failed to decode param '/%PUBLIC_URL%/favicon.ico'
    at decodeURIComponent (<anonymous>)
    at decode_param (/Users/michaelmcmillan/reproduce/my-app/node_modules/express/lib/router/layer.js:172:12)
[...]

Pay attention to the two URLs http://localhost:3000/pag and http://10.10.10.86:3000/pag. They're both missing the last character (e).

Why is this happening?
There's a lot of technical acrobatics going on when create-react-app tries to determine its URL.

I've added a failing test case to demonstrate how packages/react-dev-utils/getPublicUrlOrPath returns a path without a trailing slash (/) if the URL contains a fragment identifier. This becomes a problem later because because packages/react-scripts/scripts/start.js assumes the URL always contain a trailing slash. Instead it ends up stripping the last character from the URL.

Feedback
Also, debugging this was unnecessarily painful. Lines like the following, with three-way ternaries, make me reconsider my career options.

    return isEnvDevelopment
      ? homepage.startsWith('.')
        ? '/'
        : validHomepagePathname
      : // Some apps do not use client-side routing with pushState.
      // For these, "homepage" can be set to "." to enable relative asset paths.
      homepage.startsWith('.')
      ? homepage
      : validHomepagePathname;

This is taken from packages/react-dev-utils/getPublicUrlOrPath.

…utils returns a path without a trailing slash (/).
@facebook-github-bot
Copy link

Hi @michaelmcmillan!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@stale
Copy link

stale bot commented Mar 20, 2020

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Mar 20, 2020
@joaofranciscosantos
Copy link

#8686
This patch might fix your problem too.

@stale stale bot removed the stale label Mar 22, 2020
@michaelmcmillan
Copy link
Author

@joaofranciscosantos Nice! Maybe you should take the test I've written in this PR to make sure it works on #8686?

@stale
Copy link

stale bot commented Apr 26, 2020

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Apr 26, 2020
@stale
Copy link

stale bot commented May 1, 2020

This pull request has been automatically closed because it has not had any recent activity. If you have a question or comment, please open a new issue. Thank you for your contribution!

@stale stale bot closed this May 1, 2020
@lock lock bot locked and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants