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

feat(gatsby-transformer-remark): Allow for multiple different remark sources #7512

Merged
merged 4 commits into from
Mar 13, 2019

Conversation

alexkirsz
Copy link
Contributor

This PR allows for having more than one gatsby-transformer-remark plugin. With the addition of the filter option, the user can now decide which nodes should be handled by which plugin. For instance, with gatsby-source-filesystem, one could do the following in order to have multiple different markdown sources:

const plugins = [
  {
    resolve: `gatsby-source-filesystem`,
    options: {
      path: `${__dirname}/src/pages`,
      name: `pages`,
    },
  },
  {
    resolve: `gatsby-source-filesystem`,
    options: {
      path: `${__dirname}/src/blog`,
      name: `blog`,
    },
  },
  {
    resolve: `gatsby-transformer-remark`,
    options: {
      filter: node => node.sourceInstanceName === `pages`,
      type: `MarkdownPage`,
    },
  },
  {
    resolve: `gatsby-transformer-remark`,
    options: {
      filter: node => node.sourceInstanceName === `blog`,
      type: `BlogPost`,
    },
  }
]

I've also added a type option so that these different plugins create GraphQL types with different names.

In the above example, we could expect that our markdown pages might have different frontmatter fields than our blog entries, but having them all under the MarkdownRemark type would force us to add filters on fields in order to only retrieve a list of either of them. Having different frontmatter formats would also pollute our GraphiQL documentation.

@alexkirsz alexkirsz changed the title Allow for multiple different remark sources [gatsby-transformer-remark] Allow for multiple different remark sources Aug 21, 2018
@alexkirsz
Copy link
Contributor Author

It could make sense to adopt this kind of API for other transformers as well.

@KyleAMathews
Copy link
Contributor

Deploy preview for using-contentful failed.

Built with commit 59aadf5

https://app.netlify.com/sites/using-contentful/deploys/5b82917d4ed62f7116e8d176

@m-allanson
Copy link
Contributor

Thanks @alexkirsz 👍 this is a really useful feature - so much so, that something like it should be added to Gatsby core instead of this being implemented separately across different plugins.

It'll be particularly useful for the new Gatsby themes functionality that @ChristopherBiscardi is working on. As part of the themes work there's going to be an issue or rfc soon™️ (next week or after) to figure out exactly how this could be added to Gatsby core. Once the rfc has been written I'll add a link here.

You can check out more info on Gatsby themes here and here.

@m-allanson m-allanson added the status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged label Dec 18, 2018
@alexkirsz
Copy link
Contributor Author

@m-allanson Any news on this?

I'm not quite sure how this would play with other transformers. For instance, the yaml transformer already leverages folder names and/or file names to generate types.

@alexkirsz alexkirsz force-pushed the topics/multiple-remark-source branch from 59aadf5 to 57ec6dd Compare February 8, 2019 14:16
@alexkirsz alexkirsz force-pushed the topics/multiple-remark-source branch from 57ec6dd to c42671b Compare February 8, 2019 14:21
@alexkirsz
Copy link
Contributor Author

FWIW I've rebased on master and published this plugin as @alexkirsz/[email protected]

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

I removed bluebird and did some tests and seems to work. Nothing breaks so lets ship this!

thank you for your patience 🙏! 💪

@wardpeet wardpeet changed the title [gatsby-transformer-remark] Allow for multiple different remark sources feat(gatsby-transformer-remark): Allow for multiple different remark sources Mar 13, 2019
@wardpeet wardpeet merged commit 95155e0 into gatsbyjs:master Mar 13, 2019
@alexkirsz
Copy link
Contributor Author

Great! Thanks!

// Use Bluebird's Promise function "each" to run remark plugins serially.
await Promise.each(pluginOptions.plugins, plugin => {

await eachPromise(plugins, plugin => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't seem to work

const eachPromise = (input, iterator) =>
input.reduce(
(accumulatorPromise, nextValue) =>
accumulatorPromise.then(() => void iterator(nextValue)),
Copy link
Contributor

Choose a reason for hiding this comment

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

this void here is problematic? after removing it, it seems to work - why is it here? some context is needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Also generally - where do we stand on this? We use bluebird pretty heavily--so I'm not sure it was worth refactoring this.

But a bit tangential, if we went that way--just need to ensure we have a working alternative for sequential promises!

Copy link
Contributor

@pieh pieh Mar 14, 2019

Choose a reason for hiding this comment

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

I personally don't agree with removing dependency for the sake of removing it (at least for stuff that is only used during the builds and don't end up in production js bundles produced by webpack). If it fixes bugs or improve performance, then that's fine.

I will open PR fixing that and add some tests for this helper.

Copy link
Contributor

Choose a reason for hiding this comment

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

When utils like bluebird have maintainers with domain knowledge and it was battle-tested by thousands of developers - I will trust that more than hand rolled and not very tested approaches.

Copy link
Contributor

Choose a reason for hiding this comment

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

In any case - here's hot-fix for issue #12578 (first commit on purpose just added failing test, and second commit "fixes" the problem.

@alexkirsz I would appreciate you taking a look on this, because I imagine there was some reason for that void to be there, but I have no clue what that would be (and it did break that utility)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pieh I'm not the author of this particular piece of code, @wardpeet is (I only authored the first commit of this PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok - sorry @alexkirsz. I should pay more attention to individual commits in the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

my bady, sorry about this

wardpeet added a commit that referenced this pull request Mar 18, 2019
wardpeet added a commit that referenced this pull request Mar 18, 2019
…sources (#12639)

## Description
This change should happen in core and not in remark itself. I didn't think this through.

I should have read @m-allanson more carefully:
> Thanks @alexkirsz 👍 this is a really useful feature - so much so, that something like it should be added to Gatsby core instead of this being implemented separately across different plugins.

> It'll be particularly useful for the new Gatsby themes functionality that @ChristopherBiscardi is working on. As part of the themes work there's going to be an issue or rfc soon™️ (next week or after) to figure out exactly how this could be added to Gatsby core. Once the rfc has been written I'll add a link here.

> You can check out more info on Gatsby themes here and here.

## Related Issues
#7512
@wardpeet
Copy link
Contributor

wardpeet commented Mar 18, 2019

hey @alexkirsz I'm sorry but reverted this change (#12639) as @m-allanson was right and we should have this implementation in core.

Thanks @alexkirsz 👍 this is a really useful feature - so much so, that something like it should be added to Gatsby core instead of this being implemented separately across different plugins:

It'll be particularly useful for the new Gatsby themes functionality that @ChristopherBiscardi is working on. As part of the themes work there's going to be an issue or rfc soon™️ (next week or after) to figure out exactly how this could be added to Gatsby core. Once the rfc has been written I'll add a link here.

You can check out more info on Gatsby themes here and here.

We want you to know that we certainly want to keep you as a contributor to Gatsby, so we'd encourage you to check out our open issues.

Thanks again, and we look forward to seeing more PRs from you in the future! 💪 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants