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

Entity: Strengthen typing of getInitialState #4423

Closed
wants to merge 1 commit into from

Conversation

GavynHolt
Copy link

@GavynHolt GavynHolt commented Jul 14, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

When initializing an EntityAdapter<T> and subsequently setting the initialState using ...adapter.getInitialState({ <additional state here>}), it is possible, given the current interface, to introduce unknown variables that do not conform to the user's specified state. This could result in bugs related to mistyped variable names, resulting in confusing user error scenarios.

Closes #4422

What is the new behavior?

This small change modifies the interface to require that the input state S extends EntityState<T>, requires that the additionalState state argument conforms to an Omit version of this S type, which removes the 'id' and 'entities' properties by way of keyof EntityState<T>, and then finally, since the argument now already extends EntityState<T>, we can now simply return S.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

I have answered "Yes", but realistically, if a user of the Entity package has entered in correct code already, there will be no breaking changes. Any breaking changes should be in the form of a user now being aware of incompatible property types in their additional state, or if their State interface does not contain extends EntityState<T>.

Other information

This change has helped my place of employment from unknown state properties quite nicely, but I must say this is my very first community/open source pull request. I would be happy to take any constructive criticism or guidance on anything that I could do better here. Very excited to be contributing and hope this is of help.

Thank you, -Gavyn Holt

Copy link

netlify bot commented Jul 14, 2024

Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 9a41659
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/66933994ddf8540009c5ba34

@GavynHolt GavynHolt changed the title feat(entity): strengthen typing of getInitialState to catch invalid a… Entity: Strengthen typing of getInitialState Jul 14, 2024
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @GavynHolt .
Imho this is a useful change, but we cannot merge this now because this is a breaking change. I'll leave this open as we can revisit this on our next major version release.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Wanted to mention the above comment as a request for changes to prevent an accidental merge.

@brandonroberts
Copy link
Member

I think we should close this PR and revisit closer to the v19 release cycle for breaking changes

@timdeschryver
Copy link
Member

timdeschryver commented Jul 17, 2024

I think we should close this PR and revisit closer to the v19 release cycle for breaking changes

Sounds good!
We'll revisit this later @GavynHolt

@GavynHolt
Copy link
Author

Cool, thanks very much. Please feel free to let me know if there's anything I can do to help when the time is right.

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.

NGRX Entity, Stronger Typing on getInitialState
3 participants