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.

Sign up for GitHub

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

Jump to bottom

Convert autop package to TypeScript #61573

Closed
wants to merge 4 commits into from
Closed

Convert autop package to TypeScript #61573

wants to merge 4 commits into from

Conversation

jpstevens
Copy link
Contributor

What?

This PR converts the autop package to TypeScript.

Why?

Ensures package is fully type checked.

How?

  • converted files to .ts
  • added explicit return types for functions .ts files
  • explicitly typed variables that would otherwise be implicit any

Testing Instructions

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

@github-actions GitHub Actions
Copy link

github-actions bot commented May 10, 2024 •
edited
Loading

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 <[email protected]>
Co-authored-by: jsnajdr <[email protected]>
Co-authored-by: sirreal <[email protected]>

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

@github-actions GitHub Actions
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
jsnajdr
jsnajdr reviewed May 14, 2024
View reviewed changes
Copy link
Member

@jsnajdr jsnajdr 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 a nice improvement, thanks for working on the TS updates!

packages/autop/src/index.ts
@@ -3,7 +3,7 @@
*
* @type {RegExp}
Copy link
Member

Choose a reason for hiding this comment

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

You can remove all the JSDoc type anottations now, they are duplicate and redundant. The docgen tool that generates the Markdown documentation will retrieve the types from the code, and should generate exactly the same output.

packages/autop/src/index.ts
@@ -55,7 +55,7 @@ const htmlSplitRegex = ( () => {
*
* @return {string[]} The formatted text.
Copy link
Member

Choose a reason for hiding this comment

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

Same here, we only need to document the params, the types are on the function signature:

@param input The text which...
@return The formatted text.

packages/autop/src/index.ts
@@ -65,7 +65,7 @@ function htmlSplit( input ) {
// If the `g` flag is omitted, `index` is included.
// `htmlSplitRegex` does not have the `g` flag so we can assert it will have an index number.
// Assert `match.index` is a number.
const index = /** @type {number} */ ( match.index );
const index = match.index!;
Copy link
Member

Choose a reason for hiding this comment

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

We don't use the ! operator, here match.index as number is preferred.

packages/autop/src/index.ts
@@ -480,7 +482,7 @@ export function removep( html ) {
// Restore preserved tags.
if ( preserve.length ) {
html = html.replace( /<wp-preserve>/g, () => {
return /** @type {string} */ ( preserve.shift() );
return preserve.shift()!;
Copy link
Member

Choose a reason for hiding this comment

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

Here also as string is better.

@Mamaduka Mamaduka added [Type] Enhancement A suggestion for improvement. [Package] Autop /packages/autop and removed First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository labels May 15, 2024
@sirreal
Copy link
Member

sirreal commented Jun 13, 2024

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

@up1512001 up1512001 mentioned this pull request Jun 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsnajdr jsnajdr jsnajdr left review comments

Assignees
No one assigned
Labels
[Package] Autop /packages/autop [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jpstevens @sirreal @jsnajdr @Mamaduka