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

TypeScript: Fix and improve types for private-apis #66667

Merged
merged 3 commits into from
Nov 1, 2024

Conversation

manzoorwanijk
Copy link
Member

@manzoorwanijk manzoorwanijk commented Nov 1, 2024

What?

This PR converts private-apis package to TypeScript.

Why?

Since private-apis package is used by many other packages, converting this package to TypeScript will pave the way for fixing types on other packages like data and core-data

How?

The PR simply renamed the .js files to .ts and adds the required types to the functions etc.

Testing Instructions

  • Just verify that the build and tests are successful

Testing Instructions for Keyboard

Screenshots or screencast

Copy link

github-actions bot commented Nov 1, 2024

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

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

@gziolo gziolo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Private APIs /packages/private-apis labels Nov 1, 2024
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Thanks! We can clean up the pre-existing JSDoc types, just like in #66669.

packages/private-apis/src/implementation.ts Outdated Show resolved Hide resolved
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Thanks @manzoorwanijk 🙌

@tyxla tyxla merged commit d6cb227 into WordPress:trunk Nov 1, 2024
63 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.7 milestone Nov 1, 2024
@manzoorwanijk manzoorwanijk deleted the fix/ts-types-for-private-apis branch November 1, 2024 11:34
*/
function unlock( object ) {
function unlock( object: Record< symbol, WeakKey > ) {
Copy link
Member

Choose a reason for hiding this comment

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

A little better type would be unlock<T>(object: unknown): T because:

  • the function can safely work with any object and try to unlock it. We might run into problems with the Record type if the locked object doesn't have a compatible type. That will probably happen a lot as we migrate the poorly-typed-yet codebase.
  • we should be able to cast the return value to be an object of known type with private methods.

I predict that we'll run into these issues as we use the typed lock/unlock and will eventually come back to revisit this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in #66682

Copy link
Member Author

Choose a reason for hiding this comment

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

That signature seems to break a lot of existing poorly typed usage of lock and unlock

karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
* Convert private-apis package to TypeScript

* Convert the usage of private-apis to TypeScript

* Clean up JSDoc

Co-authored-by: manzoorwanijk <[email protected]>
Co-authored-by: tyxla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Private APIs /packages/private-apis [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants