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

Prevent path prefix from being incorrectly applied #3233

Merged
merged 10 commits into from
Dec 22, 2017

Conversation

m-allanson
Copy link
Contributor

@m-allanson m-allanson commented Dec 15, 2017

This fixes #2551 in my example repo, and it doesn't break any existing tests.

Are there any tests I can add to ensure this stays fixed?

Possibly this resolves the following issues too: #2740 and #3104.

@KyleAMathews
Copy link
Contributor

Awesome, thanks for looking into this!

There's a test file already for loader.js — probably could add a new test there.

@gatsbybot
Copy link
Collaborator

gatsbybot commented Dec 15, 2017

Deploy preview ready!

Built with commit 39b0a29

https://deploy-preview-3233--gatsbygram.netlify.com

@ghost ghost assigned KyleAMathews Dec 20, 2017
@ghost ghost added the review label Dec 20, 2017
@KyleAMathews
Copy link
Contributor

@m-allanson want to add a test or two here so we can ship it?

@m-allanson
Copy link
Contributor Author

m-allanson commented Dec 21, 2017

Yeah I was planning to dig into this again today.

I added a couple of tests before, but either my understanding is wrong (quite likely 😀) or there's another issue with path prefixes. So I need to revisit it.

@m-allanson
Copy link
Contributor Author

@KyleAMathews This is updated now, I think it fixes up an extra couple of oddities around the path prefix handling.

I've tested various path prefix combinations on a demo Gatsby site, but it'd be good to get confirmation that I've done that properly 😀

@m-allanson
Copy link
Contributor Author

Urgh the CI fails on Node 6, but passes on Node 8. I'm out of time for now, but can take another look next week if no one picks it up before then.

@KyleAMathews
Copy link
Contributor

Looks like "startsWith" is the problem — added simple polyfill from MDN.

@KyleAMathews
Copy link
Contributor

Actually polyfilling this is silly. Implementing the algorithm.

@KyleAMathews
Copy link
Contributor

Hmm something still failing in node 6.

@KyleAMathews
Copy link
Contributor

Perhaps with how Jest does the global stuff.

@m-allanson
Copy link
Contributor Author

m-allanson commented Dec 22, 2017

@KyleAMathews I was able to reproduce this in a standalone repo: https://github.com/m-allanson/jest-global-test. I've posted an issue over at facebook/jest in case it's something that can be fixed on their side.

@m-allanson
Copy link
Contributor Author

Now Travis is 💚 on Node 6 and 8.

@KyleAMathews KyleAMathews merged commit 12a9cd3 into gatsbyjs:master Dec 22, 2017
@ghost ghost removed the review label Dec 22, 2017
@KyleAMathews
Copy link
Contributor

Nice! Thanks for persevering through the weirdness :-)

@m-allanson m-allanson deleted the topics/path-prefix-flag branch December 22, 2017 20:01
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.

--prefix-paths flag is always enabled
3 participants