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

refactor: unifies spread types in their proper places #2067

Closed
wants to merge 6 commits into from

Conversation

wellwelwel
Copy link
Collaborator

@wellwelwel wellwelwel commented Jun 15, 2023

It's a large TypeScript refactoring that includes every single *.ts file in the entire project.


Why?

Many types are confusing because they are redefined several times, for example:

  • Pool types are spread and defined in index.d.ts, typings/mysql/index.d.ts and typings/mysql/lib/Pool.d.ts at the same time
    • The same for Connection, ConnectionOptions, execute, query, etc.
  • execute and query are complex and have to do several ctrl + c and v around the types
  • Sometimes a type is only present in one of them, in other as overwritten and another time it's just repeated

To consider

  • It makes new TS contributions much easier:
    • Do you want to change some type in Connection? It's simple: go to Connection.d.ts file πŸ‘¨πŸ»β€πŸ’»
  • It makes creating new TS tests more practical
  • By certain way, many past changes that have caused TS errors is because the types have been spread around

CI

  • npm run lint:code now checks for every *.ts file (including the tests)

What does this refactoring change?

In final usage, nothing. It's just a refactoring, but... πŸ€ΉπŸ»β€β™€οΈ

  • βœ… These changes resolves 100% of eslint errors in *.ts

  • βœ… Unifies the types in their proper places:

    • Pool as example: now, all typings are defined in typings/mysql/lib/Pool.d.ts
    • execute and query overloads too. Now both can be reused:
      • If you change something in type execute or query, it will automatically affect all types that need it (Connection, PoolConnection, etc.) πŸ€ΉπŸ»β€β™€οΈ
  • βœ… Increments NodeNext TypeScript compatibility by requires the .js in local imports for *.ts files

  • βœ… Resolves circular imports in every *.ts file

  • βœ… Patterns all imports as import any from './'

    • Before, it was mixed with import any = require('./')
  • ℹ️ Removes older TypeScript tests that are unused

    • Then, removes @types/mocha, @types/chai, mocha and chai dependencies

Lastly

It's the new index.d.ts:

export * from './typings/mysql/index.js';

Yes, it's everything, because each type is in its proper place and the index.d.ts just calls them πŸ₯·πŸ»βœ¨

@wellwelwel wellwelwel changed the title refactor: typescript refactor: unifies the types in their proper places Jun 15, 2023
@wellwelwel

This comment was marked as resolved.

@wellwelwel wellwelwel changed the title refactor: unifies the types in their proper places refactor: unifies spread types in their proper places Jun 15, 2023
@sidorares
Copy link
Owner

great work @wellwelwel
I just glanced over the changes I think it looks good but would be really good if someone else could take another look

@wellwelwel wellwelwel mentioned this pull request Jun 16, 2023
@wellwelwel

This comment was marked as resolved.

@wellwelwel
Copy link
Collaborator Author

wellwelwel commented Jun 17, 2023

@sidorares, I'm thinking of closing this PR.
A large change like that it's hard to get a good review.

Instead, I'm considering bringing here a Milestone that I have into a private repository where I test everything before bringing it here.

Then, reach exactly at the same place as this PR, but in stages.

Independently of closing or not, I think it's good to bring it here.
By this way, my PRs would be more easily planned and everyone would be aware of everything

@wellwelwel
Copy link
Collaborator Author

See #2072

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.

None yet

2 participants