Skip to content

Commit

Permalink
chore(www): revert amend pass context code snippet (#12084)
Browse files Browse the repository at this point in the history 
Reverts #12012 as it's actually not needed.
As per Slack discussion with @sidharthachatterjee
  • Loading branch information
@LekoArts @DSchau
LekoArts authored and DSchau committed Feb 25, 2019
1 parent 0d7449d commit 52cb180
Showing 1 changed file with 0 additions and 1 deletion.
1 change: 0 additions & 1 deletion docs/docs/creating-and-modifying-pages.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ exports.onCreatePage = ({ page, actions }) => {
createPage({
...page,
context: {
...page.context,
house: Gryffindor,
},
})
Expand Down

5 comments on commit 52cb180

@raulrpearson
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, @LekoArts and @DSchau, why is ...page.context not needed? Will context: { house: Gryffindor } on its own not overwrite the context field? Will the previous spread of ...page merge the old and new context objects?

I actually proposed #12012 because I run into that problem when using the snippet on a project and I think ...page.context was the solution (I can fish out the details if that'd be helpful)

Unrelated but now that I'm at it: shouldn't Gryffindor be quoted for the snippet to work? It's seems the snippet intended that to be a string literal.

@LekoArts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no previous context :) Using the createPage function it’s the first time you pass context to that page.

Yeah, it should be in quotes. Mind opening a PR?

@raulrpearson
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no previous context :) Using the createPage function it’s the first time you pass context to that page.

What if a different plugin is adding something to context? I think I ran into this problem on a site that was using gatsby-mdx. I've checked that codebase right now and I found they're merging frontmatter into context. Interesting 🤔 , what do you think?

I understand the docs snippet should be as simple as possible so as not to confuse people, but if this overwriting scenario is a possibility, it could create some confusion and frustration for noobs like me that aren't aware that they're overwriting.

Yeah, it should be in quotes. Mind opening a PR?

Will do! 👍

@LekoArts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

gatsby-mdx doesn't overwrite your context values as it uses lodash's merge. It only adds frontmatter to the context with the contents of the frontmatter (due to the spread operator) :)

@raulrpearson
Copy link
Contributor

Choose a reason for hiding this comment

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

gatsby-mdx doesn't overwrite your context values as it uses lodash's merge

My point is that I was overwriting what gatsby-mdx had placed in context by using this code snippet from the docs. I was loosing frontmatter. It took me a while to figure out and was a bit confused and frustrated at the time. That's why I suggested #12012.

It seems safer to merge the previous context with the new one (using lodash's merge or spreading the old context into the new one) than to assume that context is empty.

Just my two cents, by the way, not trying to antagonize 🙂

Please sign in to comment.