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

chore(docs): Add --prefix-paths option to gatsby develop docs #14319

Closed
wants to merge 1 commit into from

Conversation

jsanchez034
Copy link

Description

The gatsby-cli gatsby develop command currently supports the --prefix-paths option that gatsby build and gatsby serve supports but the option is not documented anywhere. This change adds that documentation.

## Description
The gatsby-cli `gatsby develop` command currently supports the `--prefix-paths` option that `gatsby build` and `gatsby serve` supports but the option is not documented anywhere. This change adds that documentation.
@jsanchez034 jsanchez034 requested a review from a team May 25, 2019 13:32
@leonardodino
Copy link
Contributor

leonardodino commented May 27, 2019

Thanks for your PR!

I found a issue with gatsby develop --prefix-paths, so I'm not sure it's ready for general usage.

Description of the bug:

the files under the static/ folder are not being served under the prefix.
On a production build (eg: non-root GitHub Pages) they would be prefixed.

example: /static/image.png should be served as /blog/image.png if pathPrefix === "blog". It's not doing that right now.

PS: I'm not part of Gatsby Core, so I'm not sure the bug described is the intended behavior.

@leonardodino
Copy link
Contributor

leonardodino commented May 27, 2019

linting is failing, please run one of:

  • ./node_modules/.bin/prettier --write docs/docs/gatsby-cli.md
  • or yarn format:other (should work on windows)

and push the changes 🙂

@LekoArts
Copy link
Contributor

Hey! Thanks so much for opening a pull request!

The change you’ve proposed is not going to be accepted because the CLI doesn't support this flag on develop. You can see the source code here. If you scroll down a bit you'll see that prefix-paths is only available on build/serve. Moreover, this is really working as intended as it should only be available to these two stages.

We absolutely want to have you as a contributor, so please take a look at our open issues for ideas, and please reach out to us on Twitter at @gatsbyjs with questions.

We offer pair programming sessions if you’d like to work with one of our maintainers to make your first contribution.

Thanks again, and we look forward to seeing more PRs from you in the future! 💪💜

@LekoArts LekoArts closed this May 28, 2019
@leonardodino
Copy link
Contributor

Hey @LekoArts thanks again for reviewing! 😄


It's a bit unfortunate that --prefix-paths does "works" on gatsby develop.
It prefixes the served pages. (I've only tested it with pathPrefix: '/blog')

I was researching a bit, using it to simplify the front-end architecture here, and having it officially supported would be awesome!

I was coupling it with netlify dev, so having prod/dev parity would be awesome here.
I'll have to step back if this is something that will break in the near future. 😕

@jsanchez034
Copy link
Author

@LekoArts thanks for the feedback! Although the develop cli command does not document the --prefix-paths option it does work with the the gatsby develop command as @leonardodino has stated.

It works because the --prefix-paths option ultimately makes it into the Gatsby webpack.config.js. Look at the program.prefixPaths variable here...

https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/webpack.config.js#L185-L191

I don't see any logic in webpack.config.js preventing the use of the --prefix-paths option for development. I can open a PR with an update to the develop CLI command options that you shared and the doc updates I made with this PR. What do you think?

@jsanchez034
Copy link
Author

Ah sorry I misunderstood @leonardodino I see that static assets are not being served under the pathPrefix defined. In my use case that is not important but I can see if you have assets in the static folder that is an issue. I'm thinking a change will need to be made in...
https://github.com/gatsbyjs/gatsby/blob/95f1fce1c60375090c2c787b126d385e91850566/packages/gatsby/src/commands/develop.js
to give proper support of --prefix-paths with the develop command. I will look into creating a full PR with all changes necessary plus docs. Thanks again for the feedback!

@LekoArts
Copy link
Contributor

LekoArts commented May 28, 2019

I will look into creating a full PR with all changes necessary plus docs. Thanks again for the feedback!

Thank you very much that you want to take this on but I'd kindly ask you to not to. The current implementation is working as intended, the prefix-paths should only be used with build. It's only relevant for the build (and you need to put the output into the right folder anyways).

@jsanchez034
Copy link
Author

jsanchez034 commented May 28, 2019

@LekoArts can I open an RFC to support prefix-paths with the develop command? It is critical for creating a custom preview server for a Gatsby app my company is using and I want to make sure that prefix-paths, while not documented, continues to work with the develop command going forward.

It's only relevant for the build (and you need to put the output into the right folder anyways).

I believe that the build output ( meant for production ) and the develop output should match. If not you can run into inconsistencies with path dependent data like slugs etc. which would lead to bugs that don't happen locally when developing but then do happen in production.

@DSchau
Copy link
Contributor

DSchau commented May 28, 2019

@jsanchez034 let me address a few of your questions!

can I open an RFC to support prefix-paths with the develop command

This doesn't need an RFC, but we appreciate the offer! The potential for breakage coupled with how substantial/impactful the feature will be is what constitutes the need for an RFC--and I don't think we need that here!

I believe that the build output ( meant for production ) and the develop output should match.

Fair enough, and we generally agree. It's weird to have little idiosyncrasies between development and build, but the approach is that some trade-offs are OK if they lead to e.g. a faster development experience. It would not be an ideal development experience if dev/build were exactly the same as far as speed of changes and instant feedback are concerned.

If not you can run into inconsistencies with path dependent data like slugs etc

The potential for breakage here is fairly low because the general approach is don't prefix your links and use the Link component. We do understand how it could be frustrating to have different behavior between development and build, however!

In a roundabout way--we wouldn't necessarily mind enabling this functionality, especially because it seems to already kind of work. We'd want to ensure the following:

  1. It's clearly documented
  2. It has first class support for all types of path prefixing (e.g. routes, static assets, images, etc.)
  3. It can be implemented relatively cleanly, without touching a lot of surface area

If you'd be willing to tackle this, and we can meet all the above--we'd definitely consider a PR like you're proposing.

@jsanchez034
Copy link
Author

Hi @DSchau, thank you for the detailed reply! I will look into creating a PR sometime this week with all the details you mentioned above.

@leonardodino
Copy link
Contributor

related: this is coming to create-react-app soon.

Maybe we can leave this issue open, and rename it to properly implement/test the feature.

@SamIngram96
Copy link

SamIngram96 commented Sep 10, 2019

For anyone still curious about this issue,

You can add asset prefixing to gatsby develop mode by using the onPreRenderHtml function to inject a base tag into your html. This will prefix all relative links with the href attribute so long as the attribute is a fully qualified domain name

Thank you @wardpeet

edit: this wont prefix /common.js or /socket.io/socket.io.js

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.

5 participants