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

feat(types): drop TypeScript < 4.1 #13954

Merged
merged 2 commits into from Jan 15, 2022
Merged

feat(types): drop TypeScript < 4.1 #13954

merged 2 commits into from Jan 15, 2022

Conversation

ephys
Copy link
Member

@ephys ephys commented Jan 15, 2022

Pull Request Checklist

Please make sure to review and check all of these items:

  • [no, the opposite actually] Have you added new tests to prevent regressions?
  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • [n/a] Did you update the typescript typings accordingly (if applicable)?
  • [n/a] Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Follow-up to sequelize/meetings#6 (comment)
This PR drops support for TypeScript < 4.1 and documents our TS support policy.

This release should cause a SemVer MINOR bump.

@ephys ephys added the type: typescript label Jan 15, 2022
@ephys ephys requested a review from WikiRik Jan 15, 2022
@ephys ephys self-assigned this Jan 15, 2022
@ephys ephys added this to To be released in Sequelize v6 release log Jan 15, 2022
Copy link
Member

@WikiRik WikiRik left a comment

Small suggestion for additional clarity

docs/manual/other-topics/typescript.md Outdated Show resolved Hide resolved
@WikiRik
Copy link
Member

@WikiRik WikiRik commented Jan 15, 2022

Also, do we really want this in v6?

@ephys
Copy link
Member Author

@ephys ephys commented Jan 15, 2022

This in v6 would allow us to merge #13909 in v6 which would provide an easier migration path to the model changes I'd like to make in v7 (simplying model definition to class User extends Model<User> {}).

But I am also of two minds about it

Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
@fzn0x
Copy link
Member

@fzn0x fzn0x commented Jan 15, 2022

I think it will be backward incompatible for some users right? how about adding it to v7 instead?

@ephys
Copy link
Member Author

@ephys ephys commented Jan 15, 2022

It will be incompatible for users that haven't updated TypeScript since Nov. 19 2020 yes. Which is why I'm of two minds.

On one hand, adding this to v6 would allow us to land #13909 in v6 which would be a great improvement for TS users & make migration to v7 easier. And 4.1 is more than one year old.
I also believe we should not make our TS changes SemVer major (hence the documentation update to reflect our policy).

On the other hand, tsc will report an error for users that are still on < 4.1.

But honestly, I have typings errors all the time after upgrades. Even Sequelize adding proper types for Model#toJSON completely broke the typings in my projects because my override had a different signature, so we (and others) are already causing typing breaking changes all the time in minor versions, or even patch. It's just too difficult to avoid. TypeScript itself doesn't follow SemVer (sequelize/meetings#6 (comment))

Honestly my only problem with this change is that we didn't document it earlier. But I think given it's been more than a year since 4.1, it's fine. Users that haven't updated TS since then are not going to update Sequelize either.

@WikiRik
Copy link
Member

@WikiRik WikiRik commented Jan 15, 2022

Yeah, it's more about that we have a policy now relatively close to a new major release. But I agree that with the changes we made to the typings many people that have updated to the latest version of sequelize also have a relatively new version of TS as well.

If migrating to v7 is made easier with this change I think it's fine if we do this in v6

@fzn0x
Copy link
Member

@fzn0x fzn0x commented Jan 15, 2022

If this makes our work easier for migrate typings, approve!

@ephys
Copy link
Member Author

@ephys ephys commented Jan 15, 2022

It won't make our job easier but it will allow users to massively reduce the boilerplate they need to write for TS support in v6

Current way: https://github.com/sequelize/sequelize/blob/main/docs/manual/other-topics/typescript.md#usage
Possible new way if we merge this in v6: https://github.com/sequelize/sequelize/blob/120e2447dc166d256f030dbc07928776b24aa649/docs/manual/other-topics/typescript.md#usage

Which would also make migration easier as that second way is what I'd like to make the default in v7

@ephys
Copy link
Member Author

@ephys ephys commented Jan 15, 2022

I'll merge this because there is consensus that we should support >= 4.1 in v7 ; but we can still debate whether we land it in v6 or not :)

Edit: If I understand correctly we first need to mark tests TS Typings (3.9) & TS Typings (4.0) as not required in the repository settings, which requires @sdepold?

@sdepold
Copy link
Member

@sdepold sdepold commented Jan 15, 2022

Power to the people!

it now requires either of @sdepold @ephys @WikiRik :)

@ephys
Copy link
Member Author

@ephys ephys commented Jan 15, 2022

Thanks @sdepold :')

@WikiRik
Copy link
Member

@WikiRik WikiRik commented Jan 15, 2022

I'll merge this because there is consensus that we should support >= 4.1 in v7 ; but we can still debate whether we land it in v6 or not :)

Seeing the difference in usage I think it's good to include this in v6 as well. Seeing that we have updated various typings in the recent releases of v6 already it should be fine.

Edit: If I understand correctly we first need to mark tests TS Typings (3.9) & TS Typings (4.0) as not required in the repository settings, which requires sdepold?

I updated the required checks so it requires 4.1 - 4.5 already for the main branch

@sdepold
Copy link
Member

@sdepold sdepold commented Jan 15, 2022

What's the benefit of having this in v6?

@WikiRik
Copy link
Member

@WikiRik WikiRik commented Jan 15, 2022

It won't make our job easier but it will allow users to massively reduce the boilerplate they need to write for TS support in v6

Current way: https://github.com/sequelize/sequelize/blob/main/docs/manual/other-topics/typescript.md#usage Possible new way if we merge this in v6: https://github.com/sequelize/sequelize/blob/120e2447dc166d256f030dbc07928776b24aa649/docs/manual/other-topics/typescript.md#usage

Which would also make migration easier as that second way is what I'd like to make the default in v7

@sdepold this is the benefit of having it in v6

EDIT: so not this PR specifically, but it allows us to merge other PRs to v6 as well which will accomplish at least the example quoted

@fzn0x
Copy link
Member

@fzn0x fzn0x commented Jan 15, 2022

It won't make our job easier but it will allow users to massively reduce the boilerplate they need to write for TS support in v6

Current way: https://github.com/sequelize/sequelize/blob/main/docs/manual/other-topics/typescript.md#usage Possible new way if we merge this in v6: https://github.com/sequelize/sequelize/blob/120e2447dc166d256f030dbc07928776b24aa649/docs/manual/other-topics/typescript.md#usage

Which would also make migration easier as that second way is what I'd like to make the default in v7

Cool!

@sdepold
Copy link
Member

@sdepold sdepold commented Jan 15, 2022

Ah sorry. My Mumbo jumbo brain somehow thought that this thread is about the utils conversion.

I think we should change the docs part in the commit message to something else. Maybe Feature for the lack of a better idea since docs is likely to result in a patch version

@ephys
Copy link
Member Author

@ephys ephys commented Jan 15, 2022

feat works for me. None of the others would cause a minor bump and we're not tagging it as BREAKING CHANGE

@ephys ephys changed the title docs(types): drop TypeScript < 4.1 feat(types): drop TypeScript < 4.1 Jan 15, 2022
@ephys ephys merged commit d384a87 into main Jan 15, 2022
40 of 42 checks passed
@ephys ephys deleted the feature/ephys/drop-ts-40 branch Jan 15, 2022
@sdepold sdepold moved this from To be released to Releasing in Sequelize v6 release log Jan 22, 2022
@sdepold sdepold moved this from Releasing to To be released in Sequelize v6 release log Jan 22, 2022
@sdepold sdepold moved this from To be released to Releasing in Sequelize v6 release log Jan 22, 2022
sdepold pushed a commit that referenced this issue Jan 22, 2022
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
@sdepold sdepold moved this from Releasing to Released in Sequelize v6 release log Jan 23, 2022
@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Feb 7, 2022

🎉 This PR is included in version 7.0.0-alpha.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

maramizo pushed a commit to maramizo/sequelize that referenced this issue Jun 2, 2022
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
aliatsis pushed a commit to creditiq/sequelize that referenced this issue Jun 2, 2022
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @v7 type: typescript
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants