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: preserve keys order when using getters and virtual fields #16913

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

evsar3
Copy link

@evsar3 evsar3 commented Dec 30, 2023

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

On a model that has getters and virtual fields, when serialized to JSON these fields are placed on top of the object. This PR implements sorting for the objects keys following the order they were declared on the model schema.

I have made this PR previously (#16169), but I was closed due to the PR not following some contribution guidelines. I have tried by best to make this match our guidelines and hope this get merged.

Thank you.

Happy New Year!

@WikiRik
Copy link
Member

WikiRik commented Jan 1, 2024

Thanks for working on this more. Could you add some tests?

@evsar3
Copy link
Author

evsar3 commented Jan 2, 2024

Have added the tests that verifies if the result of keys of<modelInstance>.get() is the same as the order of keys of Model.modelDefinition.rawAttributes

Not sure if this is the right location for placing this test, let me know if I should move it into another file.

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.

I'm not really a fan of this approach as it's going to be pretty expensive just for what is pretty much a cosmetic change

If we want to do this, we should iterate modelDefinition.attributes.keys first, assign any value that exists, then iterate dataValues and add any value that wasn't added yet. This would respect the definition order without needing to sort after the fact

@evsar3 evsar3 requested a review from a team as a code owner March 18, 2024 23:04
@evsar3 evsar3 requested review from ephys and WikiRik and removed request for a team March 18, 2024 23:04
@sequelize-bot sequelize-bot bot added the conflicted This PR has merge conflicts and will not be present in the list of PRs to review label Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicted This PR has merge conflicts and will not be present in the list of PRs to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants