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

Convert token-list package to TypeScript #61575

Conversation

jpstevens
Copy link
Contributor

@jpstevens jpstevens commented May 10, 2024

What?

This PR converts the token-list package to TypeScript.

Why?

Ensures package is fully type checked.

How?

  • converted files to .ts
  • explicitly type functions and incorrectly-implied variables, based on @type, @param and @return comments

Testing Instructions

  • npm run test:unit -- packages/token-list tests pass
  • npm run build:package-types returns a zero exit code

Copy link

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jpstevens <jpstevens@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @jpstevens! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label May 10, 2024
@Mamaduka Mamaduka added [Type] Enhancement A suggestion for improvement. [Package] Token List /packages/token-list and removed First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository labels May 15, 2024
@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Jun 13, 2024
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

👋 @jpstevens Thanks for this! If you'll update this PR on the latest trunk and remove types from JSDoc, we can likely give this a final review and land it.

@@ -64,7 +64,7 @@ describe( 'token-list', () => {

it( 'sets to stringified value', () => {
const list = new TokenList();
list.value = undefined;
list.value = undefined as unknown as string;
Copy link
Member

Choose a reason for hiding this comment

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

For a case like this, I'd go with expect error. We know the type is wrong and we expect to have an error reported:

Suggested change
list.value = undefined as unknown as string;
// @ts-expect-error The value should be a string, for testing we pass a "bad" value.
list.value = undefined;

@@ -4,49 +4,45 @@
* @see https://dom.spec.whatwg.org/#domtokenlist
*/
export default class TokenList {
private _currentValue: string;
private _valueAsArray: string[];

/**
* Constructs a new instance of TokenList.
*
* @param {string} initialValue Initial value to assign.
Copy link
Member

Choose a reason for hiding this comment

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

All the types that have moved into TypeScript syntax should be removed from JSDoc annotations like this:

Suggested change
* @param {string} initialValue Initial value to assign.
* @param initialValue Initial value to assign.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Package] Token List /packages/token-list [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants