Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve using elements in lists #412
base: main
Are you sure you want to change the base?
Improve using elements in lists #412
Changes from 28 commits
05cbbfe
ca36edc
0937a2e
fa730f9
f90444d
c8f0acc
bcc1074
732e652
89c4d7d
1dcaa3d
966d397
1b7d84a
5b167f1
057e1c9
b60f091
f39e3b4
2d87e95
d15b036
090e38a
9778469
0260289
d166aa6
6467a3e
bd6cc74
01675e4
2c9da83
4294b9b
8fbfa30
9ac3840
d65a3d4
964a924
938ef81
58e8a6f
5c9de6d
59bce3b
40179f7
e3c5ce4
d00c534
dadf9dd
ef78668
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this, but it also might be a good place to pump the breaks on overloading and provide an argument label or some such to disambiguate.
This would also solve the potential issue of branching implementations which could in pathological cases cause unpredictable behavior for types which adopt multiple (non-inherited) protocols.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed for the result builders in particular – so it's not that we can provide an overload that's useful – it's that we need to signal to the compiler that this is the method set, etc to use for these passing types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylebshr Creating a thread to reply to stuff, so we can talk in a thread vs top level comments:
Hard to say – there's a bunch of existing
ElementItem
usages in POS now, and they're all pretty much wrong – the API was hard to use, so there's a lot of this:But in this case the underlying value is
"formFields-element"
, aka a static string; soisEquivalent
always returns true, and the item will never re-size even when it should – which is already a foot gun and breaks things in weird ways.Identifiers? No – the list smart enough to do a "best attempt" at creating stable identifiers when there's duplicate IDs (and identifiers are already salted with the
ItemContent
type), so even without explicitly provided IDs, you'll get stable IDs across updates. Eg, if this is your list, these will be the identifiers:The main benefit to providing IDs is during mutative diffs, the list can more intelligently manage the changes.
Same thing as you'd do before with
ItemContent
/BlueprintItemContent
; you need to manually implementEquatable
orisEquivalent
, and compare the equatable parameters that you can. There are some clever things we can do here to avoid needing this, but I didn't explore it: We can usemirror
to find equatable parameters and compare themThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the right direction, and only question if we should require the presence of an id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I subsequently understood that this was meant to improve the builder experience. I have mixed feelings about that but do think it matches the standard folks have come to expect from SwiftUI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 2c / thoughts are that, since you only really need to provide an ID as an optimization (eg if your list is changing a lot, or you're reading data back out – eg for reordering), the framework can already make a pretty good guess what you meant, so requiring it isn't super useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And indeed, people get the ID wrong pretty often – eg just passing a
UUID()
every time – which is very destabilizing / a perf issue)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey its been over a year, I am going back on this and requiring the ID! If we see it really getting abused again, we can remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, hmm, this does make quite a few places less ergonomic. I'm going to ruminate on this one a bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I went back on this again. Not doing an ID.