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: support additional YAML files #124

Merged

Conversation

aaronamm
Copy link
Contributor

Changes proposed

  • The main change of this PR is setting gatsby-transformer-yaml's option to support additional YAML files.
    • If the current processed file is sidebar.yml, expose as sidebarSidebarItems node and keep the current behavior, no breaking change if other people are about to update a newer version with this merged PR.
  • Add dataPath configuration and set a default value to data folder's name.
  • Add using-yaml-files.md of how to use YAML files in the project.
  • Rename imported SEO component to Seo to fix Pascal case naming warning.

My concern

  • If a user update gatsby-theme-docs-core to version 3.1.0 (minor), they will get a data folder automatically.
    However if one does not commit a data folder to a repository because it is an empty folder, this causes gatsby-source-filesystem throw an error
  • I think after this PR get merged, we need to update @rocketseat/gatsby-theme-docs to use @rocketseat/gatsby-theme-docs-core@3.1.0. However, I get confused because I am very new with changeset tool and don't know will it be patch or minor for a version of @rocketseat/gatsby-theme-docs project. Therefore, I leave this for you.
  • If you need me to do an addition PR to update @rocketseat/gatsby-theme-docs's version, please let me know if it will be patch or minor because I don't know if you want to keep number versions of these these two projects synced.
  • We should consider to not specify typeName of sidebar.yml file and use a default typeName of gatsby-transformer-yaml.
    This is because we can remove our custom typeName function and simplify our code. However, this causes a breaking change and is not backward compatibility for existing projects that use @Rocketseat.

Fixes #120

Additional context

N/A

…ditional YAML files

Configure gatsby-transformer-yaml's options to support additional YAML files, not only sitebar.yml.

jpedroschmitz#120
@changeset-bot
Copy link

changeset-bot bot commented Jan 10, 2022

🦋 Changeset detected

Latest commit: 1ce871d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@rocketseat/gatsby-theme-docs-core Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@aaronamm
Copy link
Contributor Author

@jpedroschmitz Is it possible for you to review this PR, I kind of need this feature. Thank you so much.

@jpedroschmitz
Copy link
Owner

jpedroschmitz commented Jan 18, 2022

Hey @aaronamm, sorry for taking so long to review it. A lot of stuff is going on right now.

I haven't worked with Gatsby in a long time, so I'm not sure about the code itself.

About the data folder, maybe we should rename it to yamlFiles or something like that. I think data is confusing.

--

The changeset will manage all the dependencies updates around the project! And minor is fine =D

Let me know what you think about it!

@aaronamm aaronamm force-pushed the support-additional-yaml-files branch from 4fe2b10 to 98c2100 Compare January 30, 2022 16:45
@aaronamm
Copy link
Contributor Author

@jpedroschmitz Thanks for your code review and sorry for late reply. I have changed "data" folder to "yamlFiles" already.
However, I am very new for changeset tool. I made a new commit as a refactor commit without a new changeset file.

I tested this change and it works without any issues. If everything is okay, please could you merge this PR.
Thanks.

Copy link
Owner

@jpedroschmitz jpedroschmitz left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this @aaronamm!

I did just a few small changes, but the rest is looking good!

And also thanks for adding docs for the new feature! Really important =D

@jpedroschmitz jpedroschmitz changed the title Support additional YAML files feat: support additional YAML files Jan 31, 2022
@jpedroschmitz jpedroschmitz merged commit c4f8d21 into jpedroschmitz:main Jan 31, 2022
@github-actions github-actions bot mentioned this pull request Jan 31, 2022
typeName: ({ node, object, isArray }) => {
/* eslint-enable no-unused-vars */

typeName: ({ node, isArray }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpedroschmitz Thanks for updating. This is good. It is object destruction, not a parameter list.

if (node.sourceInstanceName === `config`) {
return `SidebarItems`;
}

// Fallback to the existing algorithm from gatsby-transformer-yaml plugin.
// https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-transformer-yaml/src/gatsby-node.js#L22-L28
if (node.internal.type !== `File`) {
return _.upperFirst(_.camelCase(`${node.internal.type} Yaml`));
return upperFirst(camelCase(`${node.internal.type} Yaml`));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is can be a good PR for Gatsby project.

@@ -11,7 +11,6 @@ module.exports = {
themeColor: `#8257E6`,
basePath: `/`,
},
flags: { PRESERVE_WEBPACK_CACHE: true },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is option useful for speed up compilation steps. Please let me know why did you remove it. Thanks.

Copy link
Owner

Choose a reason for hiding this comment

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

It's no longer required on Gatsby 4, as it's the default. There was a warning about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpedroschmitz Nice, thanks you so much for letting me know. I should have updated with Gatsby 4. ^^.

@aaronamm
Copy link
Contributor Author

aaronamm commented Feb 1, 2022

@jpedroschmitz Thank you so much for merging this PR and help me improve the code.
However, is it possible for you to answer my question a last comment.
Thanks again.

@jpedroschmitz
Copy link
Owner

I still couldn't publish it because my NPM token is no longer valid :/

Since I changed companies, I'm no longer in Rocketseat NPM organization.

I'll try to get a new one or rename the package.

@aaronamm
Copy link
Contributor Author

aaronamm commented Feb 1, 2022

@jpedroschmitz Thank you for you supporting. I appreciate your help.

@jpedroschmitz
Copy link
Owner

Hey, I was able to get a new token! The release was published!

Thanks for your patience ❤️

@aaronamm
Copy link
Contributor Author

@jpedroschmitz Thank you so much.

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.

Please make the project exposes multiple YAML files to GraphQL nodes.
2 participants