-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feature: add TypeScript types to the blocks package #48604
base: trunk
Are you sure you want to change the base?
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @johnhooks! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
Thank you for starting to work on it!
@adamziel or @dmsnell might have good insights on splitting the effort into smaller PRs. They gradually added typings to other big packages, |
Thank you, I've been studying the data package. Actually one of the biggest questions I have is about how to correctly type |
3939afc
to
6a82d78
Compare
cc: @sirreal I've generally fell into a stance against major changes like this to TypeScript because of some of the fundamental challenges, as it seems you found with IMO a more achievable approach is flipping all of the source files in a package to TS and starting out extremely permissive (even with abundant My assessment for this mismatch between the JS code as written and the TS types is that we tend to have the actual types for an object distributed through seemingly random parts of the codebase. A property may not have any mention except for in a file deep in some seemingly-unrelated file in a different part of the repo and yet application developers rely on that. It's extremely hard to find all these places and then keep the types updated as people are rapidly extending the interfaces in other unrelated development. As @gziolo noted I spent a lot of time and labor with @adamziel and @sarayourfriend trying to figure out how to add the benefit of the types without blocking legitimate development and it felt near impossible to do. the things that avoided getting in the way tended to be somewhat useless from a type standpoint because we'd have types like But that's kind of what we have to do in the project to avoid being an obstacle for development as long as the TypeScript code interfaces with JS code. Without being 100% TS we lack the tools to uncover all the indirect places that the core code is modified. Even then, we have the further problem that some of the important code is outside the repo, found in plugin code. There was an idea after WCEU last year to try a different approach, which was to analyze things like the selector and actions files and programmatically generate types. While |
@dmsnell I see exactly what you mean. I agree that adding
Some code outside the
I understand. Honestly my biggest goal is to learn Gutenberg to the core. I have typically done so in previous projects by exploring the code and this is taking me deep inside how the Blocks work. Even if the entire package isn't updated to TypeScript, the types being completed in package/blocks/src/types/index.ts could be helpful and at least complete when finished. Do you think there would be benefit to that? And updates to the JSDoc comments? Oh also another big goal was to start finding out who the TypeScript people are 😄. It seemed like it was going to be hard to do in Slack. |
Also I'm totally willing to take what I discover and update the |
@johnhooks I definitely think that starting at the leaf-nodes, so to speak, is the more achievable approach. that is, for example, typing individual selectors with JSDoc types. a whole-hearted switch from
A good example might be |
2fdffad
to
2453989
Compare
UPDATE: I am walking back from an attempt of full conversion to TypeScript after some consideration and advice. I am working to integrate types similar to |
1aa8625
to
a680d85
Compare
There are still |
@johnhooks thanks for the update. I see the PR is still blocked by a new release of |
@tyxla other than the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions to fill in the missing documentation. Hope that helps!
d188000
to
6266acf
Compare
6266acf
to
51373e3
Compare
Update: The changes to CC: @dmsnell, @gziolo, @noahtallen, and @tyxla |
I will try and get to a more thorough review of this later; for now I need to put it on the TODO list. |
I wanted to reference a recently opened issue #53605 where some challenges with typing |
@johnhooks Thanks for the update, I'd be very happy to see initiatives to improve TypeScript support and integration like this one move ahead. Thank you! |
51373e3
to
38f6ffd
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
* @property {Component} edit Component rendering an element to | ||
* manipulate the attributes of a block | ||
* in the context of an editor. | ||
* @property {WPBlockVariation[]} [variations] The list of block variations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variations
is a property I don't have on the BlockType
.
Thanks @tyxla for helping fill in the gaps. Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @noahtallen. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
BlockStyle, | ||
BlockType, | ||
} from '../types'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyBJacobs would you recommend explicitly typing out the action types here and using the union of Action
as the return type of all the action creators?
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @noahtallen. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
1 similar comment
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @noahtallen. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Use React.ComponentType and React.ReactElement instead of WPComponent and WPElement. See: [packages/element/src/react.js](https://github.com/WordPress/gutenberg/blob/ca2ee4d6fa8c7044cd1d3045c0985a77659f44fa/packages/element/src/react.js#L37-L47)
be7d5eb
to
3e01dc1
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @noahtallen. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Updated my profile so I don't get these anymore :) |
What?
Add TypeScript types to
@wordpress/blocks
Supersedes and closes #43686.
This PR is build on the work already done on
@types/wordpress__blocks
by the following:Jon Surrell https://github.com/sirreal
Dennis Snell https://github.com/dmsnell
Tomasz Tunik https://github.com/tomasztunik
Lucio Giannotta https://github.com/sunyatasattva
Bas Tolen https://github.com/bastolen
Why?
In the conversion regarding @ryanwelcher's PR #43686, there seemed some consense that having the type definitions separate from the source is brittle. I agree and want to help work toward the solving the issue.
How?
This PR adds types to
@wordpress/blocks
by defining them in TypeScript files and applying them to functions using the JSDoc syntax. The end result is to build type definitions directly from the source code.Testing Instructions
This PR makes no edits to working code. The type checker and the existing test should provided adequate testing, I think? Opinions on how to further test the types are very welcome.
Notes
This is what I consider a first step towards converting the package to TypeScript. At the moment this does not type check the JavaScript files of the package. In this form the types are more helpful to develops of packages that rely on
@wordpress/blocks
than the package itself. Though I would argue having JSDocs pointing to documented types is also a big step in the right direction.