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

feat(gatsby-plugin-netlify): Allow status codes in redirects (#11255) #11484

Merged
merged 7 commits into from
Feb 5, 2019
Merged

feat(gatsby-plugin-netlify): Allow status codes in redirects (#11255) #11484

merged 7 commits into from
Feb 5, 2019

Conversation

joostdecock
Copy link
Contributor

Description

This addresses #11255. A request to implement support for status
codes other that 301/302 for this plugin, as Netlify also supports
other status codes.

@sidharthachatterjee suggested I submitted a PR
with my proposed changes, and this is that PR.

It adds one option to the createRedirect call which is statusCode.
You can use it to manually set the status code. If it's not set, the
status code will be determined by the isPermanent option as before
(301 if true or 302 if not (the default) ).

I've also updated the README to document this new feature.

Related Issues

Fixes #11255

This addresses #11255. A request to implement support for status
codes other that 301/302 for this plugin, as Netlify also supports
other status codes.

@sidharthachatterjee suggested I submitted a PR
with my proposed changes, and this is that PR.

It adds one option to the `createRedirect` call which is `statusCode`.
You can use it to manually set the status code. If it's not set, the
status code will be determined by the `isPermanent` option as before
(`301` if `true` or `302` if not (the default) ).

I've also updated the README to document this new feature.
@sidharthachatterjee
Copy link
Contributor

Thank you so much, @joostdecock

Could you please rebase over master once? I think Prettier behaviour changed from the last version and that's why we're seeing the linting errors

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Just some small documentation stuff, otherwise LGTM!

packages/gatsby-plugin-netlify/README.md Outdated Show resolved Hide resolved
packages/gatsby-plugin-netlify/README.md Outdated Show resolved Hide resolved
@joostdecock
Copy link
Contributor Author

Could you please rebase over master once? I think Prettier behaviour changed from the last version and that's why we're seeing the linting errors

My bad. I hand-edited the table markup to make it prettier but it seems prettier didn't agree.

@sidharthachatterjee sidharthachatterjee changed the title feat(gatsby-plugin-netlify) Allow status codes in redirects (#11255) feat(gatsby-plugin-netlify): Allow status codes in redirects (#11255) Feb 1, 2019
@KyleAMathews
Copy link
Contributor

We'd need to update the docs too https://www.gatsbyjs.org/docs/actions/#createRedirect

@joostdecock
Copy link
Contributor Author

We'd need to update the docs too https://www.gatsbyjs.org/docs/actions/#createRedirect

I assumed this documented the Gatsby createRedirect API? This adds one option (statusCode) which is specific to the plugin. But the plugin already has the force option, which is also specific to the plugin, and not documented in the Gatsby API.

Which is why I figured this should only be documented in the plugin README. If you disagree, I'd be happy to take a stab at updating the documentation, but I assumed it was intentionally not included in the Gatsby API docs.

If you can advice on the best course of action, I'd be happy to work on it.

@joostdecock
Copy link
Contributor Author

If you can advice on the best course of action, I'd be happy to work on it.

As this has gone cold a bit, I figured I'd let you know that I'm still happy to update the documentation. But before doing so, someone would need to decide whether it's needed or not (see my earlier question).

@sidharthachatterjee
Copy link
Contributor

I agree with @joostdecock that updating the createRedirect docs shouldn't be necessary since this only affects the netlify-plugin

@pieh
Copy link
Contributor

pieh commented Feb 4, 2019

So this is tough decision to make, because gatsby core itself can't do anything with statusCode - it needs to rely on plugins like gatsby-plugin-netlify to use this information.

There is at least one other plugin (that I'm aware of) - gatsby-plugin-s3 that does create hosting redirect rules ( https://github.com/jariz/gatsby-plugin-s3/blob/master/src/gatsby-node.ts#L30 ). On one hand it would be great if this would be in gatsby spec so those plugins are interchangeable, but on other adding this during v2 release will potentially make it so some plugin do use this field and some don't and can be confusing to users.

@jariz As maintainer of gatsby-plugin-s3, what are your thoughts?

@pieh
Copy link
Contributor

pieh commented Feb 4, 2019

My gut feel right now is to merge as-is and we can discuss adding statusCode to gatsby's createRedirect docs in the meantime to not block this feature for gatsby-plugin-netlify, but will wait at least a day in case folks feel otherwise

@KyleAMathews
Copy link
Contributor

A plugin that requires a field creates a defacto standard. If we're going to add support to the plug-in, we should document it.

@jariz
Copy link
Contributor

jariz commented Feb 4, 2019

Thanks for the mention @pieh.
Considering we have multiple plugins being able to handle these statuscodes (such as plugin-netlify & plugin-s3), I think it's very vital to document it. (why not? it's backwards compatible anyway)
It is being said that plugin-netlify will be the only one implementing this, but obviously I'm also interested in implementing this myself if it gives more freedom to the users, and so will any future plugin maintainers of other hosting solutions likely.

@pieh
Copy link
Contributor

pieh commented Feb 4, 2019

@jariz You obviously can implement this as well, but my worry is that there are other plugins (that I'm not aware of) that implement that feature.

But I think You raised very valid point that we should document it in createRedirect docs, because this is the place where plugin maintainer will get their information - I just got convinced :)

@joostdecock We are in alignment here that we should document this option in https://www.gatsbyjs.org/docs/actions/#createRedirect

@joostdecock joostdecock requested a review from a team as a code owner February 5, 2019 08:45
@joostdecock
Copy link
Contributor Author

joostdecock commented Feb 5, 2019

We are in alignment here that we should document this option in https://www.gatsbyjs.org/docs/actions/#createRedirect

That's great. I took a first stab and updated the (inline) docs. In've added the statusCode option (that I added in the PR) but also the force option that was already in the Netlify plugin.

@jariz I've added your S3 plugin as an example, but it would be nice if you could check whether the way I phrased things works out for you.

Thanks in advance for your feedback.

(edited for typos)

@jariz
Copy link
Contributor

jariz commented Feb 5, 2019

@joostdecock lgtm!

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

LGTM too! Thanks @joostdecock for implementation and @jariz for feedback!

@pieh pieh merged commit 024f6f4 into gatsbyjs:master Feb 5, 2019
@gatsbot
Copy link

gatsbot bot commented Feb 5, 2019

Holy buckets, @joostdecock — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@joostdecock joostdecock deleted the plugin-netlify-statuscodes branch February 5, 2019 18:45
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