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

fix(gatsby-plugin-create-client-paths): set matchPath correctly #15014

Merged
merged 3 commits into from
Jun 21, 2019

Conversation

sidharthachatterjee
Copy link
Contributor

Array.some short circuits early whereas we need to run through all to get the best match

Fixes #15011

@sidharthachatterjee sidharthachatterjee requested a review from a team as a code owner June 21, 2019 07:37
@sidharthachatterjee sidharthachatterjee added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jun 21, 2019
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job! I added a few nits but nothing blocking! 👍

@@ -18,7 +18,7 @@ exports.onCreatePage = ({ page, store, actions }, { prefixes }) => {
return resolve()
}

prefixes.some(prefix => {
prefixes.forEach(prefix => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

github commmenting is dumb 💤

can you also remove the return true & return false from this function as forEach doesn't care.

I'm also thinking if we should drop the promise. What use has it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promise shouldn't be necessary since createPage is an action creator and therefore not async

@gatsbybot gatsbybot merged commit e69800b into master Jun 21, 2019
@wardpeet wardpeet deleted the fix/plugin-create-client-paths-match-path branch June 21, 2019 08:01
johno pushed a commit to johno/gatsby that referenced this pull request Jul 17, 2019
…byjs#15014)

* Add (failing) tests

* Don't short circuit early

* Fix README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gatsby-plugin-create-client-paths: Sets matchPath incorrectly for client side path entry
3 participants