Component Props Array & Map. Stabilize reference fetching #6144
Conversation
Dependency Review✅ No vulnerabilities or OpenSSF Scorecard issues found.Scanned FilesNone |
98a202d to
67b455c
Compare
67b455c to
6b51468
Compare
2ef7278 to
883613a
Compare
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const av = data.attributeValues[avId]!; | ||
| const prop = av.propId ? data.props[av.propId] : undefined; | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const childrenIds = data.treeInfo[avId]!.children; |
There was a problem hiding this comment.
ok - seeing this, I haven't looked more closely, but I could smash the prop and av together in a single struct with only what we need here. I assume documentation, link, widget, etc, could live on the AV itself. Let me know what you think!
There was a problem hiding this comment.
Nope, this is fine I think. FWIW the "Tree" you see here isn't the prop tree you made. And its not exactly the av tree Jacob made either. I essentially took the tree info and populated it with the objects, and children objects, all the way down. Its useful to have it this way for rendering and fuzzy searching
There was a problem hiding this comment.
yeah I know - I just mean we can return this exact shape in the MV itself.
| :path="props.attributeTree.attributeValue.path ?? ''" | ||
| :kind="props.attributeTree.prop?.widgetKind" | ||
| :value="props.attributeTree.attributeValue.value?.toString() ?? ''" | ||
| :canDelete="props.attributeTree.isBuildable" |
There was a problem hiding this comment.
is this metadata about the av something you might want in the MV itself? For example, isChildOfMapOrArray or something to that effect
There was a problem hiding this comment.
eh... six of one, half a dozen of the other...
| attributeTree: AttrTree; | ||
| }>(); | ||
|
|
||
| // this handles objects and arrays but not empty arrays | ||
| const hasChildren = computed(() => props.attributeTree.children.length > 0); | ||
| const hasChildren = computed(() => { |
There was a problem hiding this comment.
same thing here - this could be in the MV
app/web/src/newhotness/layout_components/ComponentAttribute.vue
Outdated
Show resolved
Hide resolved
| // this is a choice, we could send through objects that don't match the types | ||
| // and potentially have something drawn on the screen—but that seems worse | ||
| // for the possible side-effects | ||
| if (hasReferenceError) return -1; |
There was a problem hiding this comment.
would love to know more about how this error happens (or could happen) in the first place.
There was a problem hiding this comment.
In terms of the "model of the program", its pretty straightforward why this could happen. A bundle of patches are not guaranteed to land on the client. For example:
- player A makes a new schema variant
- player B missed that patch (networking blip packets never arrive, offline for a moment, etc)
- player A makes a component for that variant
- player B receives a Component patch with a reference to a variant it doesn't have, throw mjolnir to have it returned to you
Why it's happening to me, locally, I'm not entirely sure, just that it was. We can still dive into it, this code just handles the error cases—it doesn't remove them. Either I was doing something dumb, and I've fixed it, or the problem remains and we're handling it.
There was a problem hiding this comment.
note that, even if player B in that scenario, was offline, we detect it, they come back online, and we cold start which would get them what they missed. Its still a race. Cold start doesn't block. They could be right on the page making the query with the miss while cold start is still happening...
… beginning of a cold start
883613a to
e50a831
Compare
First commit: Array and Map "add/remove" is now working! PSA for @vbustamante and @jkeiser when I delete one element of an array, they all get deleted, seems to be an API bug?
Second commit: Big stabilization of web worker reference checking and returning