[v2] fix build with pathPrefix

[nihgwu]

No description provided.

[m-allanson]

Hmm these tests fail if I run them locally. Not sure why they're passing on Travis but failing here:

Summary of all failing tests
 FAIL  packages/gatsby/cache-dir/__tests__/loader.js
  ● Loader path prefixing › Path prefix present but not enabled

    ReferenceError: __PREFIX_PATHS__ is not defined

      175 |
      176 |   addPagesArray: newPages => {
    > 177 |     if (__PREFIX_PATHS__) {
      178 |       pathPrefix = `${__PATH_PREFIX__}/`
      179 |     }
      180 |     findPage = pageFinderFactory(newPages, pathPrefix.slice(0, -1))

      at Object.addPagesArray (packages/gatsby/cache-dir/loader.js:177:5)
      at Object.test (packages/gatsby/cache-dir/__tests__/loader.js:141:12)

  ● Loader path prefixing › Path prefix missing but enabled

    ReferenceError: __PATH_PREFIX__ is not defined

      176 |   addPagesArray: newPages => {
      177 |     if (__PREFIX_PATHS__) {
    > 178 |       pathPrefix = `${__PATH_PREFIX__}/`
      179 |     }
      180 |     findPage = pageFinderFactory(newPages, pathPrefix.slice(0, -1))
      181 |   },

      at Object.addPagesArray (packages/gatsby/cache-dir/loader.js:178:23)
      at Object.test (packages/gatsby/cache-dir/__tests__/loader.js:161:12)


Test Suites: 1 failed, 75 passed, 76 total
Tests:       2 failed, 3 skipped, 542 passed, 547 total

Running on Node v8.11.1 and macOS 10.13.4.

@nihgwu what is the error that this is solving?

[nihgwu]

@m-allanson As I mentioned, build is broken if I set pathPrefix in gatsby-config but don't include the --prefixPath option

[nihgwu]

@m-allanson seems I'm wrong with the Nodev6 issue, can you help me to revise the test cases, the key point is that __PREFIX_PATHS__ should be always boolean instead of undefined

[m-allanson]

As I mentioned, build is broken if I set pathPrefix in gatsby-config but don't include the --prefixPath option

I missed where you mentioned that - but it sounds like it's this issue again? Which was originally fixed in https://github.com/gatsbyjs/gatsby/pull/3233/files.

Could you push up a small repro site?

Thinking about it... maybe we can make this logic much simpler? In webpack.config.js we could remove __PREFIX_PATHS__ and just define __PATH_PREFIX__ depending on program.prefixPaths:

-           __PREFIX_PATHS__: program.prefixPaths,
-           __PATH_PREFIX__: JSON.stringify(store.getState().config.pathPrefix),
+           __PATH_PREFIX__: program.prefixPaths ? JSON.stringify(store.getState().config.pathPrefix) : ``,

Then completely remove __PREFIX_PATHS__ from the rest of Gatsby:

- let pathPrefix = `/`
- if (__PREFIX_PATHS__) {
-   pathPrefix = __PATH_PREFIX__ + `/`
- }
+ let pathPrefix = __PATH_PREFIX__ + `/`

This should make the tests much simpler too, and means we can drop that odd Node 6 workaround. Thoughts?

Edit: removed Gatsby v1 code

[nihgwu]

@m-allanson 👍 that would be much better, I have too many such checks there

[nihgwu]

how about simply

__PATH_PREFIX__: JSON.stringify((program.prefixPaths ? store.getState().config.pathPrefix : ``) + `/`),

we append / everywhere and then trim it via pathPrefix.slice(0, -1), that's to verbose, we should consolidate to all with or without /

[nihgwu]

also see my comments here #5228 (comment)

We should resolve that quickly, the pathPrefix breaks again and again, like #5178

[m-allanson]

👍 to getting this working reliably.

how about simply

__PATH_PREFIX__: JSON.stringify((program.prefixPaths ? store.getState().config.pathPrefix : ``) + `/`),

This sounds good but would be a breaking change for plugins that depend on the existing behaviour?

Let's do this in two steps - first we can update webpack.config.js and remove all the checks for __PREFIX_PATHS__.

Then in a later PR we can do a breaking change to always add /. This later PR could also add a check to automatically strip extra / at the end of a pathPrefix. Which means we can also remove this requirement from the pathPrefix docs: // Note: it must *not* have a trailing slash..

[nihgwu]

@m-allanson I removed __PATH_PREFIX__ completely, and have the following convention which keeps the former behavior:

• __PATH_PREFIX__ for pathPrefix without /
• pathPrefix for __PATH_PREFIX__ with /

[nihgwu]

tried for a while, still don't know how to play with the tests :(

[nihgwu]

@m-allanson I think we don't need to append / any longer, but I got another breaking change idea, move withPrefix to gatsby from gatsby-link, as we use basename now, there is no reason we use __PATH_PREFIX__ in gatsby-link anymore

[nihgwu]

all the tests pass, I removed some unnecessary tests from gatsby-link

[m-allanson]

This is a great simplification of all the path prefix handling code, thank you! It should make this much more robust in the future.

I noticed errors when testing this on a local site, but I think it's just one fix in the webpack config, see my comment there.

I got another breaking change idea

Could you open an issue for this? I'd love to see what other folks think of it too.

[m-allanson]

` ` should include quotes, e.g. JSON.stringify(``)

See the note about direct text replacement in the webpack docs. I think this should fix the netlify deploys too.

fyi I noticed that changes to webpack.config.js weren't being copied over by gatsby-dev-cli, not sure why...

[m-allanson]

Rename this to With custom pathPrefix?

[nihgwu]

I don't think so, the result is with custom pathPrefix, but actually we are testing with pathPrefix in gatsby-config and build with --prefix-paths

[m-allanson]

👍

[m-allanson]

And this one to With default pathPrefix?

[nihgwu]

Same here

[m-allanson]

👍

[m-allanson]

👍 👍 👍

[pieh]

Nice! Yeah, this is a lot simpler. One thing to remember is take care of that https://github.com/gatsbyjs/gatsby/blob/v2/examples/using-remark-copy-linked-files/src/layouts/index.js#L14-L17 when updating that example

[nihgwu]

@pieh nice catch, I didn't search the examples folder

[m-allanson]

There could be other plugins checking for __PREFIX_PATHS__ too, so I guess this should be mentioned in the v2 migration guide. I can add a note to the migration guide issue.

[nihgwu]

@m-allanson I don't think so, using __PREFIX_PATHS__ is not documented and for sure would be broken, we should always use withPrefix to achieve that need

[nihgwu]

I'll update the other parts too

[nihgwu]

@pieh I simply remove the related code from that example as it's unnecessary now

[pieh]

@nihgwu you don't have to worry about that example in context of this PR - that example wasn't yet updated to v2, so removing that part I linked will be part of updating example to v2

---edit:
I guess I was too late with my comment :) Thanks for actually doing it

[m-allanson]

This has merge conflicts because I merged #5637 :/ It's not too tricky to fix though:

mv packages/gatsby/cache-dir packages/gatsby/src/cache-dir

And then commit changes. Then you should be able to merge the v2 branch in. See example at https://github.com/gatsbyjs/gatsby/commits/nihgwu-neo/fix-build

[pieh]

This has merge conflicts because I merged #5637 :/

Fixed those conflicts here, so if this need any more work, pull changes from your repository.

[nihgwu]

anything else should I do?

[m-allanson]

anything else should I do?

This looks all good. Thanks again!

[nihgwu]

@m-allanson thank you inviting me 😄