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

Allow Key and KeyOrValue to be treated as Data #1571

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

rjwittams
Copy link
Collaborator

To let them be part of a data argument.

@cmyr
Copy link
Member

cmyr commented Feb 8, 2021

This feels iffy to me, because for a Key the actual value in the env that the key resolves to could change with the key still being equal for the purposes of data, and that seems like a possible source of confusion?

@rjwittams
Copy link
Collaborator Author

The use case for this is passing KeyOrValues around e.g

  • picking things in one widget, and passing them to another widget via construction in view switcher - common in widget demos/examples).
  • in a complex composite (e.g table) you may have a bunch of construction time config (including things to resolve from Env) between child widgets that you promote into app data using Scope.

The general form of the problem you are worried about is trivially true for all "Data" - someone changes something outside of data that we depend on in some calculation that we cache the result of (in local widget state or data), and we don't observe for changes in the non data item.

f(Data, not-data) -> something we can't track with just .same()...

Anyone resolving KeyOrValues should either be checking if the env has changed in update and re-resolving or resolving them at the point of use. I agree that probably people don't do this everywhere. I think that is more of a generic issue with env than anything else.

It seems unintuitive to not be able to trivially pass around arguments that we give to widgets in data - meaning anyone who needs to do so will end up wrapping them up and manually implementing Data).

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

This makes sense; I think what I would generally do in this sort of case is something like just annotate the fields with #[data(ignore)], but if you do want to know, (for the sake of Data) when the key has actually changed then this makes sense to me.

@rjwittams
Copy link
Collaborator Author

Yeah, in the table case I do want to know if the keys have changed, so I can reresolve them. But I do have to check for env changes separately as its another input to the resolution. I can see how trying to make this kind of thing 'just work' does lead towards a full on dependency graph ala SwiftUI/Adapton... erk!

@cmyr
Copy link
Member

cmyr commented Feb 8, 2021

yea, I've definitely been struggling to find an architectural middle-ground.

@rjwittams rjwittams merged commit 00bd2b3 into linebender:master Feb 8, 2021
@rjwittams rjwittams deleted the kv-data branch February 8, 2021 15: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.

2 participants