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

Refactor javascript statesystem #4530

Merged
merged 38 commits into from
Apr 12, 2023
Merged

Refactor javascript statesystem #4530

merged 38 commits into from
Apr 12, 2023

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Mar 31, 2023

This pull request refactors the javascript state system.

After reviewing multiple existing state systems in #4456, I decided litState and @lit-app/state where the most promising.

But while working with it, I found the typescript support of litState to be lacking, while @lit-app/state has great typescript support but lacks the concept of a StateRecorder that litState has which helps avoiding a lot of boilerplate code.

Thus I decided to write a custom system that combines both of the above. It consists of the following components:

  • State a class that inherits from EventTarget. It can be subscribed to and will dispatch an event to all subscribers when any of its properties change. It also record every read of it's properties to the stateRecorder. All credits to @lit-app/state for the idea to use EventTarget to avoid reinventing an event system.
  • stateProperty a decorator used for properties in the State class. This decorator overwrites the get and set methods to make sure events get dispatched. Thanks to lit for giving me examples of decorators that support both the babel and the typescript definition.
  • stateRecorder instance. A global instance that records every property of a state that gets read between it's start and finish. Credits to litState.
  • StateController a ReactiveController that uses the stateRecorder to track all stateProperties that are read during a render cycle. It then subscribes to the relevant states to trigger an update of its host every time one of those stateProperties changes. Thanks to @lit-app/state for introducing me to controllers for this usecase Reactive controllers
  • StateEvent a custom event that signifies state changes. Credits @lit-app/state
  • StateMap extends State and implement Map. It notifies subscribers of key changes as if it where stateProperties.

I used this new system to refactor all state systems that I introduced over the past year. This includes the combination of PubSub and the statemixin, SearchQuery and SorrQuery, which all had their own kind of subscription system.

  • Tests were added

Closes #4456.

@jorg-vr jorg-vr added the chore Repository/build/dependency maintenance label Mar 31, 2023
@jorg-vr jorg-vr self-assigned this Mar 31, 2023
@jorg-vr jorg-vr marked this pull request as ready for review April 6, 2023 07:49
@jorg-vr jorg-vr requested a review from a team as a code owner April 6, 2023 07:49
@jorg-vr jorg-vr requested review from bmesuere and chvp and removed request for a team April 6, 2023 07:49
@bmesuere bmesuere requested a review from niknetniko April 6, 2023 08:50
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

Looks good in general, a fewer smaller nitpicks here and there.

I have two (larger) questions:

app/assets/javascripts/state/state_system/State.ts Outdated Show resolved Hide resolved
app/assets/javascripts/state/state_system/State.ts Outdated Show resolved Hide resolved
app/assets/javascripts/state/state_system/State.ts Outdated Show resolved Hide resolved
app/assets/javascripts/state/state_system/State.ts Outdated Show resolved Hide resolved
app/assets/javascripts/state/state_system/StateRecorder.ts Outdated Show resolved Hide resolved
app/assets/javascripts/state/state_system/StateRecorder.ts Outdated Show resolved Hide resolved
app/assets/javascripts/state/state_system/StateRecorder.ts Outdated Show resolved Hide resolved
@jorg-vr jorg-vr marked this pull request as draft April 6, 2023 13:50
@jorg-vr
Copy link
Contributor Author

jorg-vr commented Apr 7, 2023

On Typescript:

The cause of the decorator issue is ts-jest. See kulshekhar/ts-jest#1155

Hi, if runtime react-native uses Babel, it won’t be possible to achieve the same behavior as ts-jest because ts-jest uses typescript compiler API. To achieve the same behavior as runtime, the only way is using only babel for ts transformation + tsc for typechecking.

I think the solution here would using only jest (not ts-jest), with babel compilation.
Right now the tests use the Typescript version of the code, while the browser uses the babel compiled version.

Moving to typescript 5 is also not possible. Because of ts-jest/lit. The types provided by lit still match the old typescript decorator types, which do not match with the new typescript definition. This causes ts-jest to throw errors...

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Apr 7, 2023

  • reminder for me to double check page changes when the queryParams get reset (a map clear happens)

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Apr 11, 2023

On copying code from litState:
I went through the code again. The greatest match was the staterecorder, which I have now given a rewrite to improve the quality. Eg. using Set instead of arrays and avoiding carrying around null pointers.

I think right now we have deviated enough of the original implementation to avoid any copyright infringements.

@jorg-vr jorg-vr marked this pull request as ready for review April 11, 2023 08:51
@jorg-vr jorg-vr requested a review from niknetniko April 11, 2023 08:51
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

nice job! I prefer this style over the previous one. It is much clearer that you are calling state functions now in the component classes.

Base automatically changed from feature/question-threading to develop April 11, 2023 12:06
@jorg-vr jorg-vr merged commit cecd69c into develop Apr 12, 2023
@jorg-vr jorg-vr deleted the chore/replace-state-system branch April 12, 2023 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Repository/build/dependency maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor javascript store/state system
4 participants