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

Constrain BaseController state to be valid JSON #366

Merged
merged 2 commits into from
Feb 26, 2021

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Feb 25, 2021

The BaseController state is now constrained to valid JSON. Anything that can't be serialized (e.g. functions) or gets mutated during
serialization (e.g. undefined gets converted to null) is disallowed.

This should prevent an entire class of bugs resulting from unexpected changes when serializing state as JSON. For example, we serialize state when sending it over postMessage, and when persisting the state.

@Gudahtt
Copy link
Member Author

Gudahtt commented Feb 25, 2021

Depends upon #365

Base automatically changed from use-unknown-over-any to develop February 25, 2021 19:31
The BaseController state is now constrained to valid JSON. Anything
that can't be serialized (e.g. functions) or gets mutated during
serialization (e.g. `undefined` gets converted to `null`) is
disallowed.

This should prevent an entire class of bugs resulting from unexpected
changes when serializing state as JSON. For example, we serialize state
when sending it over `postMessage`, and when persisting the state.
@Gudahtt Gudahtt force-pushed the constrain-base-controller-state-to-be-json branch from 7862dc2 to 91e9715 Compare February 25, 2021 19:42
@Gudahtt Gudahtt marked this pull request as ready for review February 25, 2021 19:42
@Gudahtt Gudahtt requested a review from a team as a code owner February 25, 2021 19:42
@Gudahtt
Copy link
Member Author

Gudahtt commented Feb 26, 2021

Note that I haven't added any test cases for this type, as testing that types work properly can be a bit challenging. But the comment I got this type from did include a TypeScript playground link will a comprehensive set of tests.

I had intended to look into something like tsd later on to test our types.

Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

LGTM! Dope!

@brad-decker
Copy link
Contributor

that type is ingenious.

@Gudahtt Gudahtt merged commit ec176a8 into develop Feb 26, 2021
@Gudahtt Gudahtt deleted the constrain-base-controller-state-to-be-json branch February 26, 2021 16:37
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The BaseController state is now constrained to valid JSON. Anything
that can't be serialized (e.g. functions) or gets mutated during
serialization (e.g. `undefined` gets converted to `null`) is
disallowed.

This should prevent an entire class of bugs resulting from unexpected
changes when serializing state as JSON. For example, we serialize state
when sending it over `postMessage`, and when persisting the state.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The BaseController state is now constrained to valid JSON. Anything
that can't be serialized (e.g. functions) or gets mutated during
serialization (e.g. `undefined` gets converted to `null`) is
disallowed.

This should prevent an entire class of bugs resulting from unexpected
changes when serializing state as JSON. For example, we serialize state
when sending it over `postMessage`, and when persisting the state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants