search

sequelize / meetings

Repository that contains all info related to team meetings and made decisions.
MIT License
0 stars 0 forks source link

How do we approach the TypeScript conversion? #6

Open sdepold opened 2 years ago

sdepold commented 2 years ago

I have just merged sequelize/sequelize#13569.

What are the next steps?

wbourne0 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

After https://github.com/sequelize/sequelize/pull/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 commented 2 years ago

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 sequelize/meetings#5 )

sdepold commented 2 years ago

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

WikiRik commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

ephys commented 2 years ago

New issue regarding the migration https://github.com/sequelize/sequelize/issues/13811 :(

sdepold commented 2 years ago

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 commented 2 years ago

I vote to do exactly that.

WikiRik commented 2 years ago

Agree with the proposal.

Two questions though;

sdepold commented 2 years ago

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

sdepold commented 2 years ago

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 commented 2 years ago

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: https://github.com/sequelize/sequelize/commit/98ad525ce008b7db3d68985818976fda08e70b67

sdepold commented 2 years ago

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 commented 2 years ago

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

ephys commented 2 years ago

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 https://github.com/sequelize/sequelize/pull/13782 and https://github.com/sequelize/sequelize/pull/13909

WikiRik commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

sdepold commented 2 years ago

FWIW: I'm fine with any solution.

ephys commented 2 years ago

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 (https://github.com/microsoft/TypeScript/issues/14116#issuecomment-280592571)

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 commented 2 years ago

Regarding DataType migration:

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

sdepold commented 2 years ago

What would be the very next steps afterwards?

New tests should be written in TypeScript (.ts)

sdepold commented 2 years ago

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

ephys commented 2 years ago

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

ephys commented 2 years ago

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 commented 2 years ago

What's still missing right now?

sdepold commented 2 years ago

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

WikiRik commented 1 year ago

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:

WikiRik commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Update on the TypeScript migration;

WikiRik commented 7 months ago

Update on the TypeScript migration;