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

refactor(gatsby): Introduce built-in GraphQL types #19951

Merged
merged 3 commits into from
Dec 9, 2019
Merged

Conversation

vladar
Copy link
Contributor

@vladar vladar commented Dec 5, 2019

Description

Before this PR the File Node type in Gatsby was inferred from example files present in the site. This caused issues when users deleted example files - things just broke.

This PR is a step forward to having types defined explicitly when possible instead of inference.

Documentation

This is an internal refactoring which shouldn't affect end-users. It uses createSchemaCustomization API under the hood.

Related Issues

Closes #18804

@vladar vladar requested a review from a team as a code owner December 5, 2019 11:28

exports.createSchemaCustomization = ({ actions }) => {
actions.createTypes(require(`../../schema/types/built-in-types`)())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if internal-data-bridge is the right place for this. Open to suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this from internal-data-bridge to schema/index.js. This has a side effect that now those types show up in some test snapshots.

const fileType = `
type File implements Node @infer {
sourceInstanceName: String
absolutePath: String
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all those really nullable? I think many (?) of those might not be. Not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's how gatsby-plugin-schema-snapshot printed them. I guess we can change it for the next major but for now, we should probably stay consistent with the current version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nullable to non-nullable shouldn't be a breaking change if they actually can't be null, I think it's a good idea to have less nullable fields if it's possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

So most stats are an output of fs.stat. We should check out TS signature for that and assign non-nullness accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this.

We have our own typings in gatsby-source-filesystem: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-filesystem/index.d.ts#L58-L96

Interestingly sourceInstanceName is also non-null. @pieh you mentioned it could be null?

@vladar vladar removed the status: WIP label Dec 6, 2019
@vladar vladar changed the title refactor(gatsby): Use schema customization for built-in types refactor(gatsby): Introduce built-in GraphQL types Dec 9, 2019
@vladar vladar added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Dec 9, 2019
@gatsbybot gatsbybot merged commit c732247 into master Dec 9, 2019
@delete-merged-branch delete-merged-branch bot deleted the vladar/gh-18804 branch December 9, 2019 13:51
mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
* refactor(gatsby): Use schema customization for built-in types (`File` and `Directory` at the moment)

* Move built-in types from internal-data-bridge to schema/index

* One more field nullability change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GraphQL schema for File
3 participants