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

How do we approach the TypeScript conversion? #6

Open
sdepold opened this issue Nov 19, 2021 · 35 comments
Open

How do we approach the TypeScript conversion? #6

sdepold opened this issue Nov 19, 2021 · 35 comments

Comments

@sdepold
Copy link
Member

sdepold commented Nov 19, 2021

I have just merged sequelize/sequelize#13569.

What are the next steps?

@wbourne0
Copy link
Member

I think we should start with core files then move outwards, otherwise when we convert a core file to TS we might have to change the files which depend on it.

@sdepold
Copy link
Member Author

sdepold commented Nov 24, 2021

I'm happy with any decision you guys make. Will be happy to merge things since it shouldn't really affect anything (other than making things better)

@ephys
Copy link
Member

ephys commented Dec 21, 2021

After sequelize/sequelize#13805, @WikiRik and I are starting to think we should postpone the TypeScript migration to v7. We've received a couple of issues regarding unexpected breaking changes in v6.

So far all of them are caused by users accessing the internal APIs (/lib). Some TypeScript breakage can also happen if we remove files from /types due to IDEs accidentally importing types from the wrong file. I have that issue myself:

Screenshot 2021-12-21 at 18 52 32

So our options are:

I would opt for option 2 because option 3 is a lot of work and option 1 is already causing users to open issues:

@WikiRik
Copy link
Member

WikiRik commented Dec 21, 2021

If I remember correctly we decided to try to do the TypeScript migration in v6 since we expected to work around the breaking changes. Seeing the number of downloads on v5 (200k on 5.22.4 in the last week) people are hesitant to upgrade to a new major version so we decided to move v7 to when we have more breaking changes ready. So this will not be an easy decision (also linked to #5 )

@sdepold
Copy link
Member Author

sdepold commented Dec 21, 2021

TBH I have no problem at all having the ts changes in v7 and let the people decide if they want to migrate.

@WikiRik
Copy link
Member

WikiRik commented Dec 21, 2021

TBH I have no problem at all having the ts changes in v7 and let the people decide if they want to migrate.

Follow-up (for during the meeting) would be if we want to release v7 as soon as TypeScript migration is done and do other breaking changes in v8 or do we want to still accumulate breaking changes before we release v7?

@ephys
Copy link
Member

ephys commented Dec 21, 2021

I hope it's ok to put this here before the meeting but: I think it's better to accumulate breaking changes that bring new features or improve existing ones, to give an incentive to upgrade

@sdepold
Copy link
Member Author

sdepold commented Dec 21, 2021

I guess I would accumulate all the interesting changes and continuously release alpha builds for v7.

@ephys
Copy link
Member

ephys commented Dec 23, 2021

New issue regarding the migration sequelize/sequelize#13811 :(

@sdepold
Copy link
Member Author

sdepold commented Dec 23, 2021

Proposal:

  1. We focus on v7 now. Aka v6 is completed and people should eventually migrate to v7.
  2. Treat main as prime development branch for v7 alpha
  3. We merge main to v7 every once in a while to release alpha version automatically
  4. We semi-manually port important PRs to v6 by either cherry-picking or copying the changes over to the v6 branch (could also be done through PRs targeting v6)
  5. Once the TS migration is done, we could release a first beta version of v7 to npm.
  6. We introduce @sequelize/core afterwards and start splitting the dialects from the core codebase and introduce them as @sequelize/postgres (name needs discussion) etc.

What do you think. Happy to have the discussion asynchronously here.

However, I think we can all agree on the fact that we are stepping on everybody's toes when we continue the TS migration on v6.

Opinion?

Edit: This might also ease the craziness in needing a proper backwards compatibility.

@ephys
Copy link
Member

ephys commented Dec 23, 2021

I vote to do exactly that.

@WikiRik
Copy link
Member

WikiRik commented Dec 23, 2021

Agree with the proposal.

Two questions though;

  • Can bigger breaking changes be done in the v7 alphas (so during the TS migration) or do we try to keep those for until after we migrated?
  • Splitting modules will be done in the betas of v7 then? Or do we move it to v8? We could use that as promotion since v7 will then really be a big rewrite

@sdepold
Copy link
Member Author

sdepold commented Dec 23, 2021

Splitting would be part of v7 and we will only turn into 7.0.0 final when every breaking change is in.

@sdepold
Copy link
Member Author

sdepold commented Dec 23, 2021

or in other words: yes we keep breaking the alpha until we are happy with it :)

edit: btw. creating a first beta is rather optional. we can talk about it in more depths once we are done with the ts migration. we can also remain in the alpha state as long as we want.

@sdepold
Copy link
Member Author

sdepold commented Dec 23, 2021

FYI: Independently of the outcome of this conversation, I'll prep an alpha pipeline for v7. For that I force replaced the existing v7 branch and stored the former changes in v7-bak (which we can then copy over). With a bit of luck we should have a v7 alpha in a moment: sequelize/sequelize@98ad525

@sdepold
Copy link
Member Author

sdepold commented Jan 10, 2022

Q: What's the status of data types conversion?
Rik: How do we make worked on files visible?

--> Create draft PRs and list all files you are likely going to touch/convert!

@sdepold
Copy link
Member Author

sdepold commented Jan 10, 2022

Sascha: Started working on utils: https://github.com/sequelize/sequelize/pull/13782/files
Zoe: Works on sequelize/sequelize#13922
Wade: sequelize/sequelize#13799

@ephys
Copy link
Member

ephys commented Jan 13, 2022

Blocking decision: Do we drop support for TypeScript < 4.1?

TypeScript 4.1 added Key remapping in mapped types, and Template Literal Types. Both incredibly useful features to improve our typing.

4.1 is also more than a year old now. I think one year of support is enough and dropping < 4.1 would allow us to greatly reduce the boilerplate our typescript users have to go add.

Blocks sequelize/sequelize#13782 and sequelize/sequelize#13909

@WikiRik
Copy link
Member

WikiRik commented Jan 14, 2022

Feel free to make a PR to drop testing on < 4.1 and add to the docs that we support 4.1 and above and then we'll see if people disagree. But I think a support window for TS versions for at least one year is fine and I would suggest doing a minor version bump whenever we lose support for older versions of TS

@ephys
Copy link
Member

ephys commented Jan 14, 2022

I agree with the minor bumb. TypeScript itself doesn't follow semver due to the idea that any TypeScript change is a breaking change. I think we should do the same.

@sdepold
Copy link
Member Author

sdepold commented Jan 14, 2022

As in, any TS version change could contain breaking changes?

@sdepold
Copy link
Member Author

sdepold commented Jan 14, 2022

FWIW: I'm fine with any solution.

@ephys
Copy link
Member

ephys commented Jan 15, 2022

Any version of TSC could include a breaking change because as they say it very well themselves, if they fix a bug, something that was not reported as an error in your codebase is now getting reported (microsoft/TypeScript#14116 (comment))

Similarly for us, if we fix our typings / make them stricter (which we've been doing a lot recently), errors that previously went unreported are now getting reported.
Not exactly the same as dropping TS < 4.1 support but I still think we should not include TypeScript in our semver major if the change is reasonable.

I'll do a PR soon :)

@sdepold
Copy link
Member Author

sdepold commented Feb 17, 2022

Regarding DataType migration:

@allawesome497 do you want to have help with the remaining 4 conversions?

@sdepold
Copy link
Member Author

sdepold commented Feb 17, 2022

What would be the very next steps afterwards?

  • Get utils finished (somehow with the changes of the PR we see randomly failing tests more often --> needs investigation)
  • Eventually model

New tests should be written in TypeScript (.ts)

@sdepold
Copy link
Member Author

sdepold commented Feb 17, 2022

Easier associations in TS --> sequelize/sequelize#14302

@ephys
Copy link
Member

ephys commented Mar 15, 2022

I've started migrating AbstractQueryGenerator to TypeScript, I need to migrate the different query generators to be confident sequelize/sequelize#14020 doesn't break anything.

@ephys
Copy link
Member

ephys commented Mar 22, 2022

I've also started migrating the associations folder, as it's very often used by AbstractQueryGenerator. I may be going down a rabbit hole. At time of posting, I'm a third of the way there.

@sdepold
Copy link
Member Author

sdepold commented Mar 23, 2022

What's still missing right now?

  • sequelize.js
  • model.js
  • data types in progress by @allawesome497
  • query-generator and association are in progress right now (by @ephys)

@sdepold
Copy link
Member Author

sdepold commented Mar 23, 2022

data-type PR review was done yesterday. sequelize/model could be handled independently.

@WikiRik
Copy link
Member

WikiRik commented Sep 21, 2022

Copying @ephys message from Slack here;

To give an idea of where we are at: We have a PR open for the migration of DataTypes, which is an absolutely massive PR (and not close to being finished)
In order of easiest to migrate to hardest, I would say:

  • -- easy difficulty --
    • /sql-string.js
    • /model-manager.js
    • /utils/validator-extras.js
    • /instance-validator.js
    • in the dialect folders: all index.js files
      • Depends on almost all other files in the dialect folders, which are complex to type but which can be temporarily typed using .d.ts files
    • in the dialect folders: all query-interface files
      • Main issue is their dependence on query-generator, but we're slowly populating query-generator.d.ts to fill the gaps
    • most test files
      • It's lower priority but migrating our tests let us find many issues with our current typings
  • -- medium difficulty --
    • in the dialect folders: all connection-manager files
    • /hooks.js
    • /sequelize.js
  • -- super hard difficulty --
    • data types (in progress). Depends on a lot. Already 161 impacted files.
    • /model.js
      • Huge file
      • This one is going to be insanely hard. Most internal methods should be moved to model-internals.ts and typed first, then model.js can be migrated
    • in the dialect folders: all query.js files
      • Hard because depends on libraries that are not necessarily implemented. May require splitting the project into a mono-repo first to separate the dialects from core.
    • in the dialect folders: all query-generator.js files
      • Incredibly complex code
      • In the meantime, we are slowly populating query-generator.d.ts files so other files can use it with typings enabled

@WikiRik
Copy link
Member

WikiRik commented Nov 6, 2022

Update now the DataTypes PR has been merged;

In order of easiest to migrate to hardest, I would say:

  • -- easy difficulty --
    • /model-manager.js
    • /utils/validator-extras.js
    • /instance-validator.js
    • /index.js and /index.mjs
    • in the dialect folders: all query-interface files
      • Main issue is their dependence on query-generator, but we're slowly populating query-generator.d.ts to fill the gaps
    • most test files
      • It's lower priority but migrating our tests let us find many issues with our current typings
  • -- medium difficulty --
    • /sequelize.js
  • -- super hard difficulty --
    • /model.js
      • Huge file
      • This one is going to be insanely hard. Most internal methods should be moved to model-internals.ts and typed first, then model.js can be migrated
    • in the dialect folders: all query.js files
      • Hard because depends on libraries that are not necessarily implemented. May require splitting the project into a mono-repo first to separate the dialects from core.
    • in the dialect folders: all query-generator.js files
      • Incredibly complex code
      • In the meantime, we are slowly populating query-generator.d.ts files so other files can use it with typings enabled

We have also created parent classes for sequelize.js and model.js so we can migrate them slowly. This approach can also be used for other files

@WikiRik
Copy link
Member

WikiRik commented Nov 15, 2022

And to add to the message above; we haven't included various other files like build.js, sscce.js, typedoc.js and files in the dev folder. Of these the SSCCE files are the most important since they are currently broken. A commit can be referenced in the SSCCE repo as a workaround, but running a SSCCE directly in the core repo is not possible.

@WikiRik
Copy link
Member

WikiRik commented Jun 20, 2023

Update on the TypeScript migration;

  • core
    • src/index.js
    • src/index.mjs
    • src/instance-validator.js
    • src/model-manager.js (PR is raised)
    • src/model.js (src/model-typescript.ts exists for slow migration)
    • src/sequelize.js (src/sequelize-typescript.ts exists for slow migration)
    • src/decorators/legacy/index.mjs
    • src/dialects/abstract/query-generator/transaction.js
    • per dialect; src/dialects/*/query.js
    • per dialect; src/dialects/*/query-interface.js (abstract has query-interface-typescript.ts for slow migration)
    • per dialect; src/dialects/*/query-generator.js (each dialect has query-generator-typescript.ts for slow migration)
    • src/utils/validator-extras.js
  • validator-js
    • src/index.mjs
  • Additional files
    • build-packages.mjs
    • typedoc.js
    • dev/update-authors.js
    • test/register-esbuild.js

@WikiRik
Copy link
Member

WikiRik commented Feb 4, 2024

Update on the TypeScript migration;

  • core
    • src/index.js
    • src/index.mjs
    • src/instance-validator.js
    • src/model.js (src/model-typescript.ts exists for slow migration)
    • src/sequelize.js (src/sequelize-typescript.ts exists for slow migration)
    • src/decorators/legacy/index.mjs
    • per dialect; src/dialects/**/query.js
    • per dialect except sqlite; src/dialects/**/query-interface.js (abstract, db2 and postgres have query-interface-typescript.ts for slow migration)
    • per dialect; src/dialects/**/query-generator.js (each dialect has query-generator-typescript.ts for slow migration)
    • src/utils/validator-extras.js
    • test/integration/**/*.test.js
    • test/smoke/smoketests.test.js
    • test/unit/**/*.test.js
  • validator-js
    • src/index.mjs
  • Additional files
    • .eslintrc.js
    • build-packages.mjs
    • typedoc.js
    • dev/update-authors.js
    • test/register-esbuild.js
    • test/esm-named-exports.test.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants