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

Repurpose the datastores to merge values, not replace #1465

Merged
merged 5 commits into from
Dec 18, 2024

Conversation

joepavitt
Copy link
Collaborator

Description

  • Rather than replacing the latest msg in the datastore, we merge the objects, overriding new values where relevant, but persistent values from previous msg's that have not had new values set.
  • This then enables a significant clean of most widgets, such that they can rely on their respective value being available in the centralised vuex store.

Related Issue(s)

Closes #1294

@joepavitt joepavitt changed the base branch from main to 679-sync-widgets November 13, 2024 16:50
@joepavitt joepavitt changed the title 1294 data storage Repurpose the datastores to merge values, not replace Nov 13, 2024
@bartbutenaers
Copy link
Contributor

Hi @joepavitt,
Thanks for having a look at this!!
Not the most fun job to do, but hopefully it will make things more stable and easy to understand.
I see that you managed to simplify the codebase a lot.
Much appreciated!!

@joepavitt
Copy link
Collaborator Author

Still a lot more I can clean out I think, was just taking a snapshot to share progress.

@colinl
Copy link
Contributor

colinl commented Nov 13, 2024

Does this change the contents of the 'last' message replay, that is received when a page connects or refreshes?

@bartbutenaers
Copy link
Contributor

bartbutenaers commented Nov 13, 2024

@colinl,
If I understand correctly it works like this (but I have only reviewed very quickly):

  1. You inject an msg with payload "xxx".
  2. That msg is stored for the ui-node id.
  3. You inject an msg with some ui_update stuff to change some dynamic properties.
  4. That msg is merged with the previous last msg, and the ui_update stuff is removed from the merged msg before replacing the last msg by the merged msg. So this new last msg still contains a payload "xxx".
  5. You inject an msg with payload "yyy".
  6. That msg is merged with the previous last msg, and the ui_update stuff is removed from the merged msg before replacing the last msg by the merged msg. So this new last msg will contain the updated payload "yyy".

So you have 1 last msg per ui node. That last msg is continiously updated by merging new input messages into it (while ignoring all ui_update stuff).

Which means it is in fact not correct anymore to talk about the last msg, because it is in fact build by merging N successive input messages.

  • If these messages have different properties, then the merged message will contain all those properties.
  • If these messages have the same properties, then the merged message will only contain the last values for that properties (because the previous values have been overwritten).

As a result you can replay that (last) merged message, because it always contains the (last) complete state.

Although I am in a very negative mood at the moment, I can't make up any scenario at the moment that would fail with this new setup ;-)

It looks like a decent solution to me.

@colinl
Copy link
Contributor

colinl commented Nov 14, 2024

OK, so potentially this could break existing contributed nodes. I think I will probably be ok because that is basically what I do myself anyway.

@bartbutenaers
Copy link
Contributor

Not sure if it will break existing nodes. Like you say you (and I) already have added some code to workaround the data loss.

I assume there will be some obsolete code in the third-party ui nodes.

@colinl
Copy link
Contributor

colinl commented Nov 14, 2024

I assume there will be some obsolete code in the third-party ui nodes.

Yes, I should be able to simplify the code.

@colinl
Copy link
Contributor

colinl commented Nov 15, 2024

Will this affect ui-template node operation?

@joepavitt
Copy link
Collaborator Author

Will this affect ui-template node operation?

No, anything already built will work as-is, this will mostly be cleaning the underlying core nodes.

Base automatically changed from 679-sync-widgets to main December 4, 2024 16:34
# Conflicts:
#	ui/src/widgets/ui-number-input/UINumberInput.vue
#	ui/src/widgets/ui-slider/UISlider.vue
#	ui/src/widgets/ui-text-input/UITextInput.vue
#	ui/src/widgets/ui-text/UIText.vue
@joepavitt joepavitt marked this pull request as ready for review December 16, 2024 11:36
@joepavitt joepavitt requested a review from Steve-Mcl December 16, 2024 11:36
Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

LGTM. Will likely break #1237 again sigh

@Steve-Mcl Steve-Mcl merged commit 357ebba into main Dec 18, 2024
1 of 2 checks passed
@Steve-Mcl Steve-Mcl deleted the 1294-data-storage branch December 18, 2024 17:35
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.

Re-architect node data storage to only store relevant data
4 participants