Protect against undefined content in Preview #146

ax-vasquez · 2020-12-18T21:05:12Z

In Gatsby Cloud's Preview, all content changes are picked up by the extension. This means that newly-created posts will be sent to Gatsby before the user has had a chance to add all required fields. Repositories for sites meant to be in use with Gatsby Preview should protect all references to required fields so that the site can handle missing elements gracefully.

Changelog

Protect against undefined content in Preview
Sync shared dependencies to the same versions used in the userland repo
Exclude dotenv files from commits
~~Fix environment loading logic~~
- ~~Only use CONTENTFUL_DELIVERY_TOKEN when CONTENTFUL_HOST is defined~~
- Holding off on this change - We, at Gatsby, likely need to adjust our use of the environment variables - opened a dialog with product team internally to get more info here
Add prettier as dev dependency and .prettierrc file with same settings as those in the userland repo
Add format script to format code to that of the same settings used in the userland repo

axe312ger · 2020-12-23T12:17:31Z

gatsby-config.js

@@ -23,7 +23,7 @@ const contentfulConfig = {
 // To change back to the normal CDA, remove the CONTENTFUL_HOST variable from your environment.
 if (process.env.CONTENTFUL_HOST) {
  contentfulConfig.host = process.env.CONTENTFUL_HOST;
-  contentfulConfig.accessToken = process.env.CONTENTFUL_PREVIEW_ACCESS_TOKEN;
+  contentfulConfig.accessToken = process.env.CONTENTFUL_DELIVERY_TOKEN;


You invert the logic users are used to:

If you have no host, we assume you want it from CDA (content delivery)

If you set a host, we assume you set it to CPA (content preview)

Maybe change the way you set your environment variables?

Thanks! I'll bring this up with the team and see what they suggest 👍

I've reverted this change for now, but am leaving this comment unresolved until I get some feedback from our product team regarding the environment variable change.

src/components/article-preview.js

src/templates/blog-post.js

axe312ger · 2020-12-23T12:20:13Z

I'd like to have the userland version and this one aligned, at least on dependency/codestyle level.

Feel free to contact me in #gatsby-contentful in your Slack :)

(Talking about contentful-userland/gatsby-contentful-starter#90 which happened in parallel)

…repo

ax-vasquez · 2021-01-04T19:06:05Z

I'd like to have the userland version and this one aligned, at least on dependency/codestyle level.

Feel free to contact me in #gatsby-contentful in your Slack :)

(Talking about contentful-userland/gatsby-contentful-starter#90 which happened in parallel)

@axe312ger I believe I addressed all of your feedback with the exception of the environment variable question (though, I did revert the change - I'm simply trying to make sure our understanding is in alignment with how you intend for us to use them 👍 )

With that said, I've since implemented:

Optional chaining in spots that could break the Gatsby Preview when the field is undefined
Set shared dependencies to the same versions as those used in the userland repo
Add prettier as dev dependency and .prettierrc file with same settings as those in the userland repo
Add format script to format code to that of the same settings used in the userland repo

ax-vasquez · 2021-01-11T20:14:14Z

@axe312ger , could I get another review on this? 🙂

I believe I addressed all of the feedback, but will certainly be happy to make changes as necessary 👍

axe312ger

If you reduce your PR to the actually purpose of protecting from rendering undefined content, I happily give ✅

But please avoid adding extra unrelated stuff to a PR. Makes it hard to read, review and maintian.

axe312ger · 2021-01-14T12:07:55Z

bin/setup.js

@@ -1,11 +1,11 @@
-const spaceImport = require("contentful-import");


While I basically agree with single quote over double quote, please do not reformat these files within this PR.

axe312ger · 2021-01-14T12:08:15Z

bin/hello.js

@@ -1,30 +1,30 @@
-const chalk = require("chalk");


please don't change this in here, make a separate PR

axe312ger · 2021-01-14T12:08:51Z

.prettierrc

@@ -0,0 +1,6 @@
+{


its nice to add prettier, but thats unrelated to protect against undefined

Please remove these changes and make a separate PR

axe312ger · 2021-01-14T12:09:21Z

package.json

@@ -45,6 +46,7 @@
    "postinstall": "node ./bin/hello.js",
    "setup": "node ./bin/setup.js",
    "netlify:login": "netlify login",
-    "netlify:deploy": "netlify deploy -d public -p"
+    "netlify:deploy": "netlify deploy -d public -p",
+    "format": "prettier --write 'src/**/*.js' 'bin/*.js'"


Same here. All upgrades and adding prettier are unrelated to your PR

I know - I was confused as to why I needed to add styling stuff (per your comment above), but maybe I am misunderstanding?

What did you mean when you said:

I'd like to have the userland version and this one aligned, at least on dependency/codestyle level.

It sounded to me like you wanted me to implement the same code style settings as that userland repo PR, but I'm not really sure what the intent was 😅 I'll standby for feedback!

axe312ger · 2021-01-25T14:54:10Z

So if we remove the formatting of unrelated files and you settle the env variable question, lets merge this.

Feel free to contact me in slack via #gatsby-contentful channel

ax-vasquez · 2021-02-08T17:57:21Z

If you reduce your PR to the actually purpose of protecting from rendering undefined content, I happily give

But please avoid adding extra unrelated stuff to a PR. Makes it hard to read, review and maintian.

Sorry, misunderstood what you meant when you asked for us to align this PR with the codestyle of the userland repo. I simply used the settings for that project. I'll go through this soon and get it fixed.

This is the comment I am referring to:

"I'd like to have the userland version and this one aligned, at least on dependency/codestyle level."

Protect against undefined content in Preview (#146)

242638a

axe312ger suggested changes Dec 23, 2020

View reviewed changes

ax-vasquez added 2 commits January 4, 2021 12:54

Use optional chaining - Sync versions with userland repo

610460c

Add prettier and script to format code in the same style as userland …

da235f5

…repo

ax-vasquez requested a review from axe312ger January 4, 2021 19:06

axe312ger suggested changes Jan 14, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect against undefined content in Preview #146

Protect against undefined content in Preview #146

ax-vasquez commented Dec 18, 2020 •

edited

Loading

axe312ger Dec 23, 2020

axe312ger Dec 23, 2020

ax-vasquez Jan 4, 2021

ax-vasquez Jan 4, 2021

axe312ger Jan 25, 2021

axe312ger commented Dec 23, 2020 •

edited

Loading

ax-vasquez commented Jan 4, 2021 •

edited

Loading

ax-vasquez commented Jan 11, 2021

axe312ger left a comment

axe312ger Jan 14, 2021

axe312ger Jan 14, 2021

axe312ger Jan 14, 2021

axe312ger Jan 14, 2021 •

edited

Loading

ax-vasquez Feb 8, 2021

axe312ger commented Jan 25, 2021

ax-vasquez commented Feb 8, 2021

		@@ -1,11 +1,11 @@
		const spaceImport = require("contentful-import");

Protect against undefined content in Preview #146

Are you sure you want to change the base?

Protect against undefined content in Preview #146

Conversation

ax-vasquez commented Dec 18, 2020 • edited Loading

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axe312ger commented Dec 23, 2020 • edited Loading

ax-vasquez commented Jan 4, 2021 • edited Loading

ax-vasquez commented Jan 11, 2021

axe312ger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axe312ger Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axe312ger commented Jan 25, 2021

ax-vasquez commented Feb 8, 2021

ax-vasquez commented Dec 18, 2020 •

edited

Loading

axe312ger commented Dec 23, 2020 •

edited

Loading

ax-vasquez commented Jan 4, 2021 •

edited

Loading

axe312ger Jan 14, 2021 •

edited

Loading