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

fix(types): add TypeScript 4.8 compatibility #14996

Merged
merged 3 commits into from Sep 18, 2022

Conversation

bnorbi95
Copy link
Contributor

Pull Request Checklist

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

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • 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?

Description Of Change

Sequelize typings are affected in a few places by a TypeScript 4.8 breaking change (https://devblogs.microsoft.com/typescript/announcing-typescript-4-8/#unconstrained-generics-no-longer-assignable-to).

The PR brings changes from #14990 (which closes #14934) to main branch.

@ephys
Copy link
Member

ephys commented Sep 17, 2022

Thank you!

Can you remove TS 4.4 from this array, and add 4.8 at its end?
https://github.com/sequelize/sequelize/blob/main/.github/workflows/ci.yml#L116

ephys
ephys previously approved these changes Sep 17, 2022
yarn.lock Outdated Show resolved Hide resolved
@WikiRik
Copy link
Member

WikiRik commented Sep 17, 2022

Also, I think it would be better if this is a fix. But we can update the PR title before merging

@ephys
Copy link
Member

ephys commented Sep 17, 2022

Also, I think it would be better if this is a fix

Damn I should have read this before merging the other one, v6 was merged as feat

@bnorbi95 bnorbi95 changed the title feat(types): add TypeScript 4.8 compatibility fix(types): add TypeScript 4.8 compatibility Sep 17, 2022
@bnorbi95
Copy link
Contributor Author

I've updated commit message and PR title to use fix.

ephys
ephys previously approved these changes Sep 17, 2022
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that it doesn't pass the lint anymore. Can you remove as any from the src/utils/sql.ts file

@ephys
Copy link
Member

ephys commented Sep 17, 2022

I think that's going to cause older typescript versions to fail. Muting the lint error could resolve the issue

@WikiRik
Copy link
Member

WikiRik commented Sep 17, 2022

That's very possible. Muting would also be fine for now

@bnorbi95
Copy link
Contributor Author

There was an additional error related to https://devblogs.microsoft.com/typescript/announcing-typescript-4-8/#lib-d-ts-updates.

Ready to review again!

@ephys ephys merged commit 7040343 into sequelize:main Sep 18, 2022
@ephys
Copy link
Member

ephys commented Sep 18, 2022

Thank you for making this PR :)

@github-actions
Copy link
Contributor

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

The release is available on:

Your semantic-release bot 📦🚀

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.

Typescript 4.8.2 does not like the type definitions from sequelize 6.21.4
3 participants