iAPI: Refactor types and add a "Core Concepts - Using TypeScript" guide #64577
Conversation
luisherranz
added
[Type] Developer Documentation
|
Thanks for this! To others out there using |
|
Oh, true. I'll add that to the guide. Thanks, Spencer! 🙂 |
luisherranz
force-pushed
the
iapi-docs-typescript-guide
branch
from
ac1d940 to
b0a1f50
Compare
|
Size Change: -4 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
@luisherranz can you confirm the introduced Without this PRs changes, I currently see type Store = ServerStore & typeof clientStore;
export const myStore = store< Store >( 'my-store', clientStore );Just wanted to make sure I was reading the test correctly in preparation. Thanks again! |
|
Yes, some types weren't working well, so I've refactored all the types. Could you test it using the version of this pull request to see if it works for you? |
luisherranz
changed the title
|
You don't need to create a completely new template gutenberg/packages/create-block/lib/templates.js Lines 62 to 69 in 8ba6b11 |
|
Can we use variants in |
Yes, that’s exactly why I mentioned it. |
|
Oh, nice. I'll try it out. Thanks, Greg! |
|
@luisherranz ping me if you run into any issues with variants |
The types from the package |
|
I've merged trunk to address conflicts. I'll push some changes for the areas I'm working on shortly. |
Done ✅ |
| describe( 'Interactivity API', () => { | ||
| describe( 'store', () => { | ||
| it( 'dummy test', () => { | ||
| expect( true ).toBe( true ); | ||
| } ); |
There was a problem hiding this comment.
@mirka I think you mentioned "testing" types in this way where types are checked against expectations in a test file.
Will you share an example of that? There's no reason to actually run tests here, this file could be placed somewhere else and skip a lot of the jest structure. I'm wondering what the precedent here looks like. Thanks!
There was a problem hiding this comment.
I was googling how people verify TypeScript types for custom code integrations but I didn’t find anything so far. I agree it would be best to avoid Jest for stuff that TypeScript can do.
There was a problem hiding this comment.
Stuff exists for testing types, I suggested we look into existing tooling:
Rather than rolling our own type testing, let's evaluate the options listed here:
If you are just looking for a TypeScript type testing tool, use:
We're already doing some light type testing, although I think we could improve that. (Found examples I was looking for in that conversation:)
There was a problem hiding this comment.
At least for components, type testing needs are pretty simple so it still isn't worth the overhead to add a dedicated tool and have contributors learn yet another syntax. Needs may be different for other packages.
We are planning on documenting/standardizing the conventions around our type testing though (#64588), and that may allow us to remove the guise of it being a "Jest test" and make it completely static. Right now the Jest wrappers are just there to signal that they are tests — it isn't necessary if we can signal it in a different way.
There was a problem hiding this comment.
I'm wondering if @adamziel and @dmsnell did something interesting for testing entities in @wordpress/core-data.
See https://github.com/WordPress/gutenberg/tree/trunk/packages/core-data/src/entity-types.
There was a problem hiding this comment.
Nothing I can remember right now. We were mostly focused on TypeScript types and done a few type assertions to make sure things look right, but I don't think we've touched the unit tests besides improving the names and readability.
packages/interactivity/src/store.ts
Outdated
| /** | ||
| * Creates a generator that yields a promise and returns its resolved value. | ||
| * | ||
| * This utility function is used to convert promises into generators, which | ||
| * can be useful when working with asynchronous actions. | ||
| * | ||
| * @since 6.7.0 | ||
| * | ||
| * @param promise The promise to be converted into a generator. | ||
| * | ||
| * @return A generator that yields the promise and returns its resolved value. | ||
| */ | ||
| export function typed< T >( | ||
| promise: Promise< T > | ||
| ): Generator< Promise< T >, T, T > { | ||
| return ( function* () { | ||
| return yield promise; | ||
| } )(); | ||
| } |
There was a problem hiding this comment.
I have some concerns about this.
(minor) The name doesn't indicate anything meaningful, it needs improvement.
(minor) The changes in how this is supposed to be used needs an example in the JSDoc.
More importantly, this function introduces a pattern that is now exclusive to TypeScript. I believe it exists almost entirely to improve types, but its implementation certainly does things. I need to think about alternatives. Related: microsoft/TypeScript#57671, microsoft/TypeScript#43632.
Now we have two patterns to do exactly the same thing but are subtly different (one needs is wrapped in the function and needs another *):
store( 'foo', {
x: function* () {
let val;
try {
val = yield Promise.reject(1);
} catch (e) {
console.error(e);
}
console.log(val);
},
y: function* () {
let val;
try {
val = yield* I.typed(/** @type {Promise<number>} */ (Promise.reject(2)));
} catch (e) {
console.error(e);
}
console.log(val);
},
} );If we go with this approach, it seems like we should discourage the plain yield Promise.resolve(1) and rely on this function (with an improved name).
There was a problem hiding this comment.
(minor) The name doesn't indicate anything meaningful, it needs improvement.
(minor) The changes in how this is supposed to be used needs an example in the JSDoc.
I agree on both counts here 👍
However, I don't find this typed() function problematic if it's only used in TypeScript to propagate the type information.
As far as I understand, it doesn't alter the behavior of the promise that you pass to it in any way. We can teach developers to use typed() (with a better name) in their TS code, but even if used in JS, it won't cause problems. If the TS team decides to improve the types to be inferred automatically, we can deprecate this typed() utility.
There was a problem hiding this comment.
That said, I also don't think there's a rush to ship this utility if we're not sure about it 🙂
There was a problem hiding this comment.
@juanmaguitar and @sirreal, one quick question. If we move typed() with accompanying unit tests to a follow up PR, is everything else ready to go? In the documentation, the examples that use typed() would also need to get removed but they contain a note that it requires WP 6.7 so they aren't too much useful for now anyway. If we would land the rest today, then the new guide would be available in the dev docs at the same time when all the changes to packages would get published to npm (will happen overnight).
There was a problem hiding this comment.
If we move typed() with accompanying unit tests to a follow up PR, is everything else ready to go?
In my opinion that's the best plan 👍
There was a problem hiding this comment.
I'll split out a separate PR today.
There was a problem hiding this comment.
#65439 matches this branch right now, but will be used to keep the work on the typed function.
There was a problem hiding this comment.
@sirreal @gziolo now that #65439 replicates this PR
- Shouldn't Interactivity API: Add typed function to use with generators #65439 contains only changes under
packages/interactivity? - Shouldn't this PR contains only changes under
docs&packages/create-block-interactive-template?
There was a problem hiding this comment.
#65439 isn't ready, changes from this branch haven't been removed from that branch. That will be done in the future, it isn't a priority for now.
There are changes and improvements to TypeScript support in packages/interactivity that are ready to land in this PR, so it's expected that there are changes to that directory in this branch.
There was a problem hiding this comment.
Oh, I see! Thanks for the clarification.
I think it's good to go, then.
sirreal
requested review from
DAreRodz,
ajitbohra,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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. |
What?
This is a new guide for the Core Concepts section that explains how to use TypeScript to type the stores of the Interactivity API.
I also ended up moving the improvements in the types that I was making in #62400:
typedfunction to be able to use promise types within generators.Why?
It might not be immediately obvious how to use TypeScript in advanced cases, so a guide can help clarify things.
There were also some things that didn't work properly with the current types, which is why I refactored them.