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-transformer-remark] support frontmatter default values plugin option #5495

Closed
wants to merge 2 commits into from

Conversation

tsriram
Copy link
Contributor

@tsriram tsriram commented May 21, 2018

This is for #5392. I'm quite not sure if it's okay to have this option defined at 2 levels deep frontmatter --> defaultValues, but this looks good to me.

@gatsbybot
Copy link
Collaborator

Deploy preview for gatsbygram ready!

Built with commit 3ffbe16

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

@gatsbybot
Copy link
Collaborator

Deploy preview for using-drupal ready!

Built with commit 3ffbe16

https://deploy-preview-5495--using-drupal.netlify.com

@Undistraction
Copy link
Contributor

This is nice simple solution to the problem. Would be great to see this merged.

for(const key of defaultKeys) {
data.data[key] = data.data[key] || frontmatterDefaults[key]
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using lodash _.merge here? It would be less verbose and it would allow deep merge (which imo would be useful because current implementation just take care of first level frontmatter fields )

@Undistraction
Copy link
Contributor

Undistraction commented May 29, 2018

Two thoughts:

  1. Is it worth considering allowing some kind of validation as well? Once defaults are applied, the resulting node could be passed to a user-defined callback, allowing validation of data. The user could use any validation system they wanted. This would allow a single place to catch missing or invalid props that might blow up in obscure errors somewhere down the line.

  2. Current PR makes the assumption that all Markdown nodes are going to require the same default values. In the starter I'm working on there are both Article and Project nodes that have different sets of frontmatter (some fields are the same, some unique to one or other node). I think there needs to be a way of associating a node with a set of defaults. The obvious way to do this is by checking the path of the node. So rather than a fixed set of defaults, how about a set of callbacks that are passed the data and the node path? This would allow each callback to decide whether to apply the defaults based on the path.

@pieh
Copy link
Contributor

pieh commented May 29, 2018

@Undistraction Good observations.

Maybe this is something we could implement in gatsby core - possibly adding some actions that could be used in onCreateNode callback, but then we would have to solve issue of contentDigest which currently is managed by plugins, which I think is main reason for current limitation that nodes should be immutable and you shouldn't manipulate them in onCreateNode etc.

@Undistraction
Copy link
Contributor

@pieh This does feel like it belongs in the core, but I'm guessing that will involve a lot more hoops to jump through. I don't want to derail this PR with complexity and I guess the validation idea has potential to introduce a lot of complexity. However I do think it needs to support different defaults for different markdown nodes, otherwise it is very restrictive and I think its utility will be limited. If it can support different nodes it becomes really useful, and pretty much solves the issue where no field is defined on any of the nodes causing GraphQL errors because the field isn't inferred.

@KyleAMathews
Copy link
Contributor

I think if you want default values, you're better off writing a plugin that adds fields with createNodeField — it can either use a set value or a backup value. This could either be a local plugin or generalize it so it can be published to NPM.

@Undistraction
Copy link
Contributor

Undistraction commented May 29, 2018

@KyleAMathews Quick outline of API - each entry is an array containing:

  1. predicate that is passed the node and decides if the defaults in 2. should be applied.
  2. map of defaults. If predicate in 1. is true and frontmatter field is undefined, fields are created using the return value of the function that is the value of that key. Otherwise a field is created with the value of the frontmatter field.

This approach allows all fields to have default values set and provides a single location for throwing useful validation errors when a required field is missing and a default value doesn't make sense.

{
  resolve: `gatsby-remark-frontmatter-defaults`,
  options: {
    defaults: [
       isArticle, { 
         category: () => 'Uncategorised', 
         date: () => Date.now()
       },
       isProject, { 
         keywords: () => [], 
         client: () => { throw new ValidationError('You must include a client for all projects') }
       }
     ],
  },
}

@KyleAMathews
Copy link
Contributor

Looks great!

@Undistraction
Copy link
Contributor

@KyleAMathews You OK with it in the gatsby repo or would you prefer it external?

@KyleAMathews
Copy link
Contributor

External — we're trying to keep this repo for essential plugins only. The plugin will show up in https://www.gatsbyjs.org/plugins/ as long as you add gatsby-plugin as a keyword to the package.json.

@Undistraction
Copy link
Contributor

Yeah. It was only a matter of time before Gatsby popped its shirt buttons with all those plugins.

Thanks. I'll get something up tomorrow.

@Undistraction
Copy link
Contributor

Undistraction commented May 30, 2018

@KyleAMathews Am I right in thinking that hooks are called in the order that the plugins are defined in gatsby.config.js (assuming they are not async)? So if gatsby-remark-frontmatter-defaults is defined after all the other remark-related plugins it will receive a node that has had any changes added by those other plugins? Can I also assume that any user-defined hooks will always happen after plugin hooks? So if a project defines its own onCreateNode hook in its own gatsby.node.js will this always be called after any onCreateNode hooks defined by plugins. I couldn't find any docs covering this.

I'm thinking each entry needs a way to define the name of the field it will write to. That way this becomes even more powerful, because it would support transforms as well:

 defaults: [
       isArticle, { 
         keywords: [
           (keywords) => {
                name: 'tags',
                value: stringListToArray(keywords || '')
             }
         ]
       },

I also think that passing in node as a second argument opens up the possibilities so more that one markdown field can be used in determining field values:

 defaults: [
       isArticle, { 
         slug: [
           (slug, node) => {
                name: slug,
                value: slug || node.frontmatter.title
             }
         ]
       },

Or do you think this is taking it a step too far? The problem is that if the plugin is writing defaults (and can't write to the markdown directly), it has to write to node fields which are also immutable meaning that they cannot be updated later by other plugins or in the user's own gatsby.config.js. For this reason it makes sense to allow a transform - so that whatever needs to happen to write a value to the field can all happen at the same time. Or is there a better approach?

@Undistraction
Copy link
Contributor

@KyleAMathews I've created a plugin to handle creation of node fields, supporting default values, transformations and validation. I think it's pretty flexible. I'd really appreciate your feedback if you get a moment to take a look: https://github.com/Undistraction/gatsby-plugin-node-fields

@KyleAMathews
Copy link
Contributor

Nifty! Looks really powerful and useful. Only feedback is some examples would help people browsing through the README understand what it can do.

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

5 participants