-
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
Interactivity API refactor to TypeScript (utils & kebabToCamelCase) #60149
Interactivity API refactor to TypeScript (utils & kebabToCamelCase) #60149
Conversation
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 If 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. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @garridinsi! 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. |
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.
Nice improvements adding missing types and JSDoc comments.
packages/interactivity/src/utils.ts
Outdated
replaceNode = [].concat( replaceNode ); | ||
const s = replaceNode[ replaceNode.length - 1 ].nextSibling; | ||
function insert( c, r ) { | ||
function insert( c: any, r: any ) { |
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.
Optional task: it would be helpful to unfold what c
, r
and s
mean.
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.
As far as I remember,
- c is children.
- r is root.
- s is sibling.
cc @DAreRodz, correct me if I'm wrong.
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.
It's all from https://gist.github.com/developit/f4c67a2ede71dc2fab7f357f39cff28c, I think @c4rl0sbr4v0 is probably right.
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.
There is a TypeScripted version in case you need:
packages/interactivity/src/utils.ts
Outdated
_useEffect( () => { | ||
let eff = null; | ||
let isExecuting = false; | ||
|
||
/** |
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.
One line comments can be shortened to:
// Notifies the callback function to execute after the next frame.
I see now that @c4rl0sbr4v0 has an alternative proposal open for adding TypeScript types to the same package but at a larger scale: #58718. So it wil require some coordination 😀 |
Hi! Yes! @c4rl0sbr4v0 made another PR but now is outdated as of some Interactivity API changes. This PR was made on WordCamp Torrelodones 2024 Contributor Day, and @c4rl0sbr4v0 is aware of this, as he was the Core table leader |
Thanks for this work! I would like to review this but need a few days to get back to it. I see a few places we can improve and/or simplify things. |
Don't worry, I can handle that coordination :-) |
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.
Thanks for the patience @garridinsi, I've left a number of suggestions and pushed a small change. I'm happy to push some of these myself or discuss further.
55a3a5c
to
c0b25f0
Compare
c0b25f0
to
5d556c7
Compare
@sirreal I updated with all the changes you suggested. Feel free to check it again. Thanks! |
packages/interactivity/src/utils.ts
Outdated
export function withScope< | ||
Func extends ( ...args: any[] ) => Generator< any, any >, | ||
>( | ||
func: Func | ||
): ( | ||
...args: Parameters< Func > | ||
) => ReturnType< Func > extends Generator< any, infer Return > | ||
? Promise< Return > | ||
: never; | ||
export function withScope< Func extends Function >( func: Func ): Func; | ||
export function withScope( func ) { |
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.
I changed this to be a function
so I could use function overloading for the typing here.
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.
I think this is ready, thanks folks!
What?
Fixes #60147
Why?
To profit from the TypeScript features.
How?
Refactors the js files to ts and adds type annotation