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 instance.dataValues property to model.d.ts #15208

Merged
merged 7 commits into from
Nov 5, 2022
Merged

fix(types): add instance.dataValues property to model.d.ts #15208

merged 7 commits into from
Nov 5, 2022

Conversation

spearmootz
Copy link
Contributor

@spearmootz spearmootz commented Oct 31, 2022

this allows typescript to have a definition for the instance.dataValues property

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • 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?
  • Does the name of your PR follow our conventions?

Description Of Change

this allows to get typings from

const sampleInstance = SampleModel.findByPk(pk);
sampleInstance.dataValues; //This currently does not exist in typings

Todos

this allows typescript to have a definition for the instance.dataValues property
@spearmootz spearmootz changed the title Update model.d.ts fix(types) Update model.d.ts to support property dataValues Oct 31, 2022
@spearmootz spearmootz changed the title fix(types) Update model.d.ts to support property dataValues fix(types): Update model.d.ts to support property dataValues Oct 31, 2022
remove additional space
@fzn0x fzn0x added the type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. label Oct 31, 2022
@fzn0x fzn0x requested a review from ephys October 31, 2022 22:55
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.

We'll take a closer look at this later but you can add this commit to the PR to have this change supported by at least one test

src/model.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Other than deleting remove-attribute.test.js, this should be good to go I think

test/unit/model/remove-attribute.test.ts Show resolved Hide resolved
ephys
ephys previously requested changes Nov 5, 2022
test/unit/model/overwriting-builtins.test.js Outdated Show resolved Hide resolved
@WikiRik WikiRik requested a review from ephys November 5, 2022 18:47
@WikiRik WikiRik changed the title fix(types): Update model.d.ts to support property dataValues fix(types): add instance.dataValues property to model.d.ts Nov 5, 2022
@spearmootz
Copy link
Contributor Author

Just wondering, will this make it to v6? Or do I have to make the PR elsewhere?

@WikiRik WikiRik dismissed ephys’s stale review November 5, 2022 19:20

Suggestions done

@WikiRik WikiRik merged commit 81b1667 into sequelize:main Nov 5, 2022
@WikiRik
Copy link
Member

WikiRik commented Nov 5, 2022

Just wondering, will this make it to v6? Or do I have to make the PR elsewhere?

Not directly, but feel free to open a new PR towards v6. Just the change to model.d.ts wil do fine, no need to update the test

@spearmootz
Copy link
Contributor Author

@WikiRik if you cloud. its the same change

#15240

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

4 participants