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

chore: add & fix type definitions for gatsby, gatsby-link, gatsby-source-fs #13619

Merged
merged 9 commits into from
Apr 30, 2019

Conversation

orta
Copy link
Contributor

@orta orta commented Apr 25, 2019

Description

Improves the TypeScript type definitions to Gatsby, and also annotates them so that you get inline documentation + API references. Not complete, but a great start. Builds off the work by @JamesMessinger in #10897

config

Screen Shot 2019-04-24 at 9 46 53 PM

gatsby-node

Screen Shot 2019-04-24 at 10 09 26 PM

and
Screen Shot 2019-04-24 at 10 10 09 PM

Link

Screen Shot 2019-04-24 at 10 12 07 PM

@orta orta requested a review from a team as a code owner April 25, 2019 02:14
@orta orta requested a review from a team April 25, 2019 02:14
@orta orta changed the title Adds type definitions with additional documentation for Gatsby + Gatsby fs Adds type definitions with additional documentation for Gatsby + Link + Gatsby fs Apr 25, 2019
@wardpeet wardpeet self-assigned this Apr 25, 2019
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.

This is amazing! 😍😍😍

There have been some api changes. I added a bunch of comments for these things. You're great at typescript so I leave it to you to fix these properly 😛 as you'll do a much better job than me 😄

I also asked a few quetions so feel free to ignore those.

I also wonder if we should expose all interfaces, I would suggest only exporting our public API. Which means, not exporting Store, Cache, ...

packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
packages/gatsby/index.d.ts Show resolved Hide resolved
@orta
Copy link
Contributor Author

orta commented Apr 26, 2019

Perfect, I did this to learn the Gatsby API - so I'll make all those tweaks.

Re: exported interfaces, it can depend a bit - if plugins or other functions end up exposing some of these interfaces then we'll have to include them. I'm definitely not in a position to know which is/isn't eventually exposed. So I'm happy to remove thing you recommend.

Copy link
Contributor Author

@orta orta left a comment

Choose a reason for hiding this comment

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

Alright, that's your changes added and amended - 👍

CI seems to be failing due to yarn-y issues, which shouldn't be related from the looks of things?

packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
packages/gatsby/index.d.ts Outdated Show resolved Hide resolved
@stefanprobst
Copy link
Contributor

Would be cool to also add

actions.createTypes = (

* exports.createResolvers = ({ createResolvers }) => {

@wardpeet
Copy link
Contributor

thanks, @stefanprobst I added both APIs.

@orta mind going over the 3 commits I made and fix them if I made some mistakes (I ran prettier on the last one so you should be able to ignore it unless we need to add some different configs). If so I can merge this one! 🎉 if anything is missing we can iterate on this one in a follow-up PR.

Copy link
Contributor Author

@orta orta left a comment

Choose a reason for hiding this comment

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

Yep, the commits look good - and I ran it in my project with no errors 👍

nice work

packages/gatsby-source-filesystem/index.d.ts Show resolved Hide resolved
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.

thank you @orta for cleaning up these types! If you have the time it would be great to add tslint support for our definitions to make sure they are actually valid.

💪 💪

@wardpeet wardpeet changed the title Adds type definitions with additional documentation for Gatsby + Link + Gatsby fs chore: add & fix type definitions for gatsby, gatsby-link, gatsby-source-fs Apr 30, 2019
@wardpeet wardpeet merged commit ad2bc6b into gatsbyjs:master Apr 30, 2019
@wardpeet
Copy link
Contributor

@antoinerousseau
Copy link
Contributor

antoinerousseau commented May 16, 2019

createRemoteFileNode returns a Promise, not a FileSystemNode.
I'm going to open a PR.

mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
…rce-fs (gatsbyjs#13619)

Improves the TypeScript type definitions to Gatsby, and also annotates them so that you get inline documentation + API references. Not complete, but a great start.
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.

4 participants