Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Add remove site page context RFC #26

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

freiksenet
Copy link
Contributor

@freiksenet freiksenet commented Jan 25, 2019

@jlengstorf
Copy link

Would it be possible to do some massaging of the data so it stays consistent with the current API?

Some pseudo-code to illustrate:

createPage({
  // ...
  context: {
    foo: 'one',
    bar: 'two',
  }
});

As @freiksenet has described, we would store this something like:

const page = {
  // ...
  context: [
    { key: 'foo', value: 'one' },
    { key: 'bar', value: 'two' },
  ],
};

The most common way (or at least the most commonly documented way) to use context is as:

  1. Variables for GraphQL queries
  2. The pageContext prop passed to the page component

In the GraphQL variables use case, nothing really changes from the user's perspective. We continue to set the variables, and GraphQL continues to work with no changes.

For page components, this could be a huge pain for developers because every instance of pageContext.foo would need to become pageContext.find(ctx => ctx.key === 'foo') (or something similar).

But if we added a helper that rebuilt the object, it would eliminate that pain:

const props = {
  // ...
  pageContext: context.reduce((ctx, { key, value }) => ({ ...ctx, [key]: value}), {})
};

This would give us the existing pattern (e.g. pageContext.foo works). Since we're giving up type safety anyways, I think this wouldn't add any risk, but keeps the nice DX.

That way this only becomes a breaking change for an edge case where someone is manually querying pages/context, which I don't know that I've ever seen in the wild.

Does this seem reasonable? Am I missing any pitfalls?

@freiksenet
Copy link
Contributor Author

freiksenet commented Jan 31, 2019

I propose that we keep the definition of context as it is and we'll pass it as it's currently passed to variables and props. The only difference would be when you query for it via GraphQL (and when you filter by it).

@jlengstorf
Copy link

That sounds great to me, @freiksenet.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants