Interactivity API: Convert types of generator functions to async functions #62400
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
luisherranz
added
[Type] Code Quality
sirreal
reviewed
Jun 7, 2024
|
Size Change: +407 B (+0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
In the end, I ended up adding a system to test types in Gutenberg, including a npm script and a GitHub Action. This is how it works: 
https://www.loom.com/share/8394c04f3273422cbaf967db0fb42923 The tests I have added to verify that the types of the Store and the Interactivity API work correctly are still very basic, but I will add more tests to cover all possibilities before merging this pull request. |
|
Flaky tests detected in a74f64c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9451426148
|
|
Rather than rolling our own type testing, let's evaluate the options listed here:
|
|
Oh, nice. Can any of those be integrated with the existing Jest setup? |
|
I'm not directly familiar with any of them, we'd need to evaluate which is best. It looks like at least one integrates with eslint and uses special comments, so that might be something to add directly to source files and errors would appear in linting. Another seems to extend jest to check types so that would be part of unit tests. |
30 tasks
For what it's worth, in the components package we've simply been appending a "static type checks" section to our unit tests, which has been good enough for our needs. We only feel the need for type tests when there are complex conditionals involved, so not often. |
|
We'll try that. Thanks @mirka! |
|
@mirka, I have added the type tests, including a failing test, to a unit test file, but I don't see the error being reported in any way. Neither by Jest when you run the tests, nor when you do This is the file: https://github.com/WordPress/gutenberg/blob/14cc9c4b70d8d6939b4d499c0238268d4b604827/packages/interactivity/src/test/store.ts What is your workflow to see that a test is failing? Or do I have to add that unit test file somewhere? |
It looks like you're set up correctly. The type tests are static, so problems will be caught by the type checker. On CI, that runs in "Static Analysis". So if your passing test case starts to fail, or if the |
That's not what I'm seeing, indeed. I have included a test that should fail here, but as you can see in the GitHub Actions, the Static Analysis has passed correctly. It also didn't give me any warnings when committing ( |
|
I think we also need to convert the input types so that people can define their actions using asynchronous functions, and these, internally, are compatible with generators. In other words, this not only has to work with inferred types, but also with explicit types. Users should be able to define store types like this: type MyStore = {
actions: {
myAsyncAction: () => Promise<void>;
};
};
// This should be fine.
const { actions } = store<MyStore>('...', {
actions: {
*myAsyncAction() {
// ...
},
},
});
// This should also be fine.
await actions.myAsyncAction(); |
|
I just realized that I made some modifications from |
You're right, I can confirm that removing a |
sirreal
deleted the
interactivity-api-types-transform-generators-into-async-functions
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This pull request updates the store return types to have their generators converted into async functions.
This is still a work in progress, as I would like to find a way to properly test these types before merging this change.
Why?
Even though developers have to use generators to create asynchronous functions in the stores, the store proxy returns an async function, not a generator. So when those actions are used as async functions, TypeScript still considers them as generators.
The async function is created on the fly by the store proxy:
gutenberg/packages/interactivity/src/store.ts
Lines 108 to 109 in 0d1943c
How?
With a utility that checks if the type corresponds to a generator and if so, changes it to an async function recursively.