Skip to content

refactor: unifies spread types in their proper places #2075

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

Merged
merged 7 commits into from
Jun 24, 2023

Conversation

wellwelwel
Copy link
Collaborator

@wellwelwel wellwelwel commented Jun 19, 2023

This PR is the last part of the TypeScript refactoring (#2072).
As before in #2067, the final usage is unaffected by these changes.

By separating the PRs, I believe it's now much better to review the changes 🔬


Unifying the types

  • I'll use the Pool as example
  • The same idea goes for all the other types that have been spread out

All Pool types spread across the index.d.ts, typings/mysql/index.d.ts and typings/mysql/lib/Pool.d.ts have been unified in "typings/mysql/lib/Pool.d.ts".


About the approach of reusable query and execute overloads

  • I'll use the execute as example
  • The same idea goes for the query overloads

I moved all variations (overloads) of the execute around the types into "typings/mysql/lib/protocol/sequences/ExecutableBase.ts".

Pool as example is a class, not an interface, so I did the ExecutableBase as an Anonymous Class.

I chose this method because it allows to dynamic extending the QueryableBase and Emitter like a Function.

For example:

declare class Pool extends QueryableBase(ExecutableBase(EventEmitter)) { /* ... */ }

It doesn't changes the execute or query types, just where they come from.

How this works in practice:

Before
Screenshot 2023-06-19 at 08 41 25

After
Screenshot 2023-06-19 at 08 42 42


Notes

  • No types were added or removed, only unified 🙋🏻‍♂️

@wellwelwel
Copy link
Collaborator Author

wellwelwel commented Jun 23, 2023

@sidorares, if you think it's better, I can do a test to strictly check the returned types for each overload of execute and query.

These tests wouldn't be in the same way as the existing tests for the execute and query.


Since it's just a refactoring, I didn't focus on tests, but I believe this could be a safer approach.

@wellwelwel
Copy link
Collaborator Author

wellwelwel commented Jun 23, 2023

After thinking for a while, I decided to create a strict test to check each overload and its returned types of execute and query.

This will ensure that both this refactoring and any future changes to the execute and query methods are tested for variations of usage and their corresponding returns in each variation of usage 🤹🏻‍♀️

These tests aren't the same as the existing tests for the execute and query.

Now I can say that this part of the #2072 is completed.
That will make the fix and the creation of the promise-based PoolCluster typings much easier 🎉


@sidorares, in this refactoring, if approved, I would like you to handle the merge.

@sidorares sidorares merged commit 8979eb1 into sidorares:master Jun 24, 2023
@wellwelwel wellwelwel deleted the refactor-ts-2 branch June 24, 2023 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants