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

gatsby-plugin-netlify tweaks #2191

Merged
merged 3 commits into from
Sep 21, 2017

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Sep 21, 2017

  • Only write _redirects file if redirects have been declared.
  • Append to _redirects file is user has defined one already in the static folder.
  • Write header and redirects files in parallel.
  • Replaced pify package with fs-extra.

Follow-up to #2185

* Only write '_redirects' file if redirects have been declared.
* Append to '_redirects' file is user has defined one already in the 'static' folder.
* Write header and redirects files in parallel.
* Replaced 'pify' package with 'fs-extra'.
Copy link
Contributor

@pk-nb pk-nb left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the changes

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 21, 2017

No problem! Thanks for kicking things off with this plugin 😁

Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

Looks good!


return writeFile(publicFolder(`_redirects`), data.join(`\n`))
return fileExists
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with this is that files in public aren't deleted between builds so each time you build the redirects file will just grow and grow.

It seems like a fairly rare thing that someone would both want to maintain a handwritten redirects file & have this plugin generating one. Perhaps make it a plugin option? E.g. appendAutogeneratedRedirectsToStaticRedirectsFile haha or a shorter option name :-D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting. I didn't realize that b'c I've been manually clearing between builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is this: If a file exists in static/_redirects it will be copied over each time the build command is run so it's safe to append. If no file exists, we'll create one initially and then (unless the user manually clears the contents of public) that same file will be around the next time the build command is run.

If that's the case, we can just search for our "Created with gatsby-plugin-netlify" comment in order to determine if it's the file we created or not. It's a little hacky I guess, but I think it should work like:

  • Is there a file?
    • No: Write a new one.
    • Yes: Does it contain the "Created with gatsby-plugin-netlify" comment?
      • No: Then it's a user-generated file, copied from static; append to it.
      • Yes: Then it's a file we created previous; override it.

I'll push an update with this proposed change momentarily. I've tested it locally and it seems to work correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Love it! Complicated implementation > complicated UX. Seems like it should be pretty stable.

@@ -1,16 +1,25 @@
import fs from "fs"
import pify from "pify"
import { appendFile, exists, writeFile } from "fs-extra"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 to fs-extra. We use it everywhere in Gatsby.

@gatsbybot
Copy link
Collaborator

Deploy preview ready!

Built with commit 238819d71b2ce823f638359e083ee858576aa787

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

@gatsbybot
Copy link
Collaborator

gatsbybot commented Sep 21, 2017

Deploy preview ready!

Built with commit 9ba3cbb

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

Don't override user redirects. Don't contiunously append to generated redirects either.
@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 21, 2017

Okay 😄 Check out the most recent push. I think it should handle the cases we've discussed without requiring additional user configuration.

@KyleAMathews KyleAMathews merged commit 3960d09 into gatsbyjs:master Sep 21, 2017
@KyleAMathews
Copy link
Contributor

Lovely!

@bvaughn bvaughn deleted the gatsby-plugin-netlify-tweaks branch September 21, 2017 21:17
@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 21, 2017

Thanks for being so responsive, Kyle!

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.

None yet

4 participants