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): make Model.getAttributes stricter #14017

Merged
merged 7 commits into from
Feb 9, 2022
Merged

Conversation

ephys
Copy link
Member

@ephys ephys commented Jan 26, 2022

Pull Request Checklist

  • 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)?
  • 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

Follow up to #13728

The typings for Model.getAttributes now declare returning an object where the keys are the attributes declared on the model.

class User extends Model<InferAttributes<User>> {
  id: number;
  name: string;
}

// before, this would not have been reported as an error:
User.rawAttributes.iDontExist;

// Now, TS reports an error on this:
User.getAttributes().iDontExist;
// but not on this
User.getAttributes().name;

This PR deprecates Model.rawAttributes & recommends using Model.getAttributes instead. I've done this because we cannot strictly type rawAttributes.

@ephys ephys added the type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. label Jan 26, 2022
@ephys ephys self-assigned this Jan 26, 2022
@ephys ephys changed the title feat(types): make Model.getAttributes stricter feat(types): make Model.getAttributes stricter Jan 26, 2022
@WikiRik
Copy link
Member

WikiRik commented Jan 26, 2022

Do you want to deprecate this in v7 and remove in v8 or deprecate in v6 and remove in v7?

EDIT2: My phone didn't show that this was already in the v6 project so nevermind that. The expiring TODO is still nice to add

EDIT: in any case, this is a nice usage for expiring TODOs

@ephys
Copy link
Member Author

ephys commented Jan 26, 2022

I don't think we'll remove it in v7 because I think it's widely used. Maybe v8

@ephys
Copy link
Member Author

ephys commented Jan 27, 2022

I'm really happy with all the TS changes we've been making. It's nice to have an easily strongly typed project

image

Something I want to check before merging this: Do attributes have the fieldName property set. It's missing from the typings.

@wbourne0
Copy link
Member

wbourne0 commented Feb 1, 2022

Hm, a bit biased but the script I made (which I've yet to adapt for general sequelize usage) uses rawAttributes as a source of truth to generate typing since its almost always going to be more accurate than TS typing (unless we can get TS to infer everything which might be ideal, though it could have some performance implications with large codebases).

I'd also be hesitant to touch rawAttributes or getAttributes without first typing the code in dialects/ since that logic can be extremely hard to grep + varies greatly.

@ephys
Copy link
Member Author

ephys commented Feb 1, 2022

Edit: I've reworded this comment because my previous iteration was a bit hard to read

I don't think this PR would interfere with your script, you can still override rawAttributes / getAttributes (unless you're referring to rawAttributes being deprecated. but you can still do declare getAttributes(): your_better_typings)

This would bring typings closer to reality for users that are not using your script, while still letting your script provide even stronger types.

Regarding first typing dialects, note that this PR only restricts the keys of the object returned by getAttributes. ModelAttributeColumnOptions remains untouched. I think it's a safe change unless there is there a scenario where that list of keys is not accurately represented by the list of keys of TAttributes (Except the user not properly declaring their attributes obv)

unless we can get TS to infer everything which might be ideal

I don't think we'll be able to make TS infer everything. I mean we could, using even more Branded Types, but it'd be verbose. I'm not against it at all, but baby steps.

@sdepold
Copy link
Member

sdepold commented Feb 6, 2022

I think I like it :)

@ephys ephys merged commit 145ce65 into main Feb 9, 2022
@ephys ephys deleted the ephys/type-get-attributes branch February 9, 2022 10:00
@github-actions
Copy link
Contributor

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

The release is available on:

Your semantic-release bot 📦🚀

@ephys ephys mentioned this pull request Feb 24, 2022
aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
vanthome pushed a commit to vanthome/sequelize that referenced this pull request Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @v7 type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants