[Flight] Add Support for Map and Set #26933
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
sebmarkbage
force-pushed
the
flightmapset
branch
from
c062f27
to
cf2cdec
Compare
| @@ -229,6 +241,24 @@ export function processReply( | |||
| }); | |||
| return serializeFormDataReference(refId); | |||
| } | |||
| if (value instanceof Map) { | |||
| const partJSON = JSON.stringify(Array.from(value), resolveToJSON); | |||
There was a problem hiding this comment.
question: What's the purpose of the We Array.from call here? I thought we already can stringify iterables so this seems redundant. Tests are passing without it.Array.from it later anyway after we extracted the iterator so we might as well do it here since we know the value is iterable.
There was a problem hiding this comment.
It's also to avoid this being an infinite loop, since otherwise we'd just end up back here since Map is preferred over the Iterable path.
There was a problem hiding this comment.
From my local testing, dropping it here was fine, but not on Server. Or vice versa, I don't quite remember. Either way, the way you proposed makes it easier to follow.
We don't atm. Gets especially interesting for Map/Set objects since the object keys might not share references in this first PR. Technically we can't really guarantee referential identity across reloads/refreshes if they're not part of the same payload but that might be an edge case. That might be an argument against adding these until we have stronger guarantees about where referential identity is guaranteed. The underlying issue is that a Server Component can show up anywhere and it can read Server Context anywhere so the same object might actually end up having two different values. This obj would have to have two different instances to represent the two different values. There's an idea to solve for this when possible, but it might not have strong guarantees when something suspends and is fairly complicated. |
There was a problem hiding this comment.
Approving because it seems to do what you intend it to but not sure we should do this until we have our answer for shared references.
I suppose we already don't support referential identity for arrays so at least the behavior here is consistent. We also don't support cycles here but that is already true for arrays as well. I can see the view that we have a problem and we shouldn't make it worse but similarly it's convincing that this problem already exists and a solution for arrays will be a solution for Map/Set so we can safely add this without complicating our predicament. How's that for "helpful" commentary
|
I'll land this as is but I think the right place it probably to encode this in the row and not the reference after all even though it's more code I suspect it'll eventually be better since it's the right model to encode the referential identity of the map itself with the id. I learned this after implementing streams. |
This PR updates the vendored react dependencies using `pnpm sync-react` ### React upstream changes - facebook/react#27028 - facebook/react#27027 - facebook/react#27019 - facebook/react#26954 - facebook/react#26987 - facebook/react#26985 - facebook/react#26933 - facebook/react#26625 - facebook/react#27011 - facebook/react#27008 - facebook/react#26997 - facebook/react#26989 - facebook/react#26955 - facebook/react#26963 - facebook/react#26983 - facebook/react#26914 - facebook/react#26951 - facebook/react#26977 - facebook/react#26958 - facebook/react#26940 - facebook/react#26939 - facebook/react#26887 - facebook/react#26947 - facebook/react#26945 - facebook/react#26942 - facebook/react#26938 - facebook/react#26844 - facebook/react#25510 - facebook/react#26932 - facebook/react#26896 - facebook/react#26913 - facebook/react#26888 - facebook/react#26827 - facebook/react#26889 - facebook/react#26877 - facebook/react#26873 - facebook/react#26880 - facebook/react#26842 - facebook/react#26858 - facebook/react#26754 - facebook/react#26753 - facebook/react#26881 ### Related Closes #49409 (by facebook/react#26977) fix NEXT-1189 Co-authored-by: Shu Ding <g@shud.in>
Fixes #49409 ### React upstream changes - facebook/react#27045 - facebook/react#27051 - facebook/react#27032 - facebook/react#27031 - facebook/react#27029 - facebook/react#27028 - facebook/react#27027 - facebook/react#27019 - facebook/react#26954 - facebook/react#26987 - facebook/react#26985 - facebook/react#26933 - facebook/react#26625 - facebook/react#27011 - facebook/react#27008 - facebook/react#26997 - facebook/react#26989 - facebook/react#26955 - facebook/react#26963 - facebook/react#26983 - facebook/react#26914 - facebook/react#26951 - facebook/react#26977 - facebook/react#26958 - facebook/react#26940 - facebook/react#26939 - facebook/react#26887 - facebook/react#26947 - facebook/react#26945 - facebook/react#26942 - facebook/react#26938 - facebook/react#26844 - facebook/react#25510 - facebook/react#26932 - facebook/react#26896 - facebook/react#26913 - facebook/react#26888 - facebook/react#26827 - facebook/react#26889 - facebook/react#26877 - facebook/react#26873 - facebook/react#26880 - facebook/react#26842 - facebook/react#26858 - facebook/react#26754 - facebook/react#26753 - facebook/react#26881 --------- Co-authored-by: Jiachi Liu <inbox@huozhi.im>
This PR updates the vendored react dependencies using `pnpm sync-react` ### React upstream changes - facebook/react#27028 - facebook/react#27027 - facebook/react#27019 - facebook/react#26954 - facebook/react#26987 - facebook/react#26985 - facebook/react#26933 - facebook/react#26625 - facebook/react#27011 - facebook/react#27008 - facebook/react#26997 - facebook/react#26989 - facebook/react#26955 - facebook/react#26963 - facebook/react#26983 - facebook/react#26914 - facebook/react#26951 - facebook/react#26977 - facebook/react#26958 - facebook/react#26940 - facebook/react#26939 - facebook/react#26887 - facebook/react#26947 - facebook/react#26945 - facebook/react#26942 - facebook/react#26938 - facebook/react#26844 - facebook/react#25510 - facebook/react#26932 - facebook/react#26896 - facebook/react#26913 - facebook/react#26888 - facebook/react#26827 - facebook/react#26889 - facebook/react#26877 - facebook/react#26873 - facebook/react#26880 - facebook/react#26842 - facebook/react#26858 - facebook/react#26754 - facebook/react#26753 - facebook/react#26881 ### Related Closes #49409 (by facebook/react#26977) fix NEXT-1189 Co-authored-by: Shu Ding <g@shud.in>
We already support these in the sense that they're Iterable so they just get serialized as arrays. However, these are part of the Structured Clone algorithm [and should be supported](facebook#25687). The encoding is simply the same form as the Iterable, which is conveniently the same as the constructor argument. The difference is that now there's a separate reference to it. It's a bit awkward because for multiple reference to the same value, it'd be a new Map/Set instance for each reference. So to encode sharing, it needs one level of indirection with its own ID. That's not really a big deal for other types since they're inline anyway - but since this needs to be outlined it creates possibly two ids where there only needs to be one or zero. One variant would be to encode this in the row type. Another variant would be something like what we do for React Elements where they're arrays but tagged with a symbol. For simplicity I stick with the simple outlining for now.
We already support these in the sense that they're Iterable so they just get serialized as arrays. However, these are part of the Structured Clone algorithm [and should be supported](#25687). The encoding is simply the same form as the Iterable, which is conveniently the same as the constructor argument. The difference is that now there's a separate reference to it. It's a bit awkward because for multiple reference to the same value, it'd be a new Map/Set instance for each reference. So to encode sharing, it needs one level of indirection with its own ID. That's not really a big deal for other types since they're inline anyway - but since this needs to be outlined it creates possibly two ids where there only needs to be one or zero. One variant would be to encode this in the row type. Another variant would be something like what we do for React Elements where they're arrays but tagged with a symbol. For simplicity I stick with the simple outlining for now. DiffTrain build for commit a1c62b8.
We already support these in the sense that they're Iterable so they just get serialized as arrays. However, these are part of the Structured Clone algorithm and should be supported.
The encoding is simply the same form as the Iterable, which is conveniently the same as the constructor argument. The difference is that now there's a separate reference to it.
It's a bit awkward because for multiple reference to the same value, it'd be a new Map/Set instance for each reference. So to encode sharing, it needs one level of indirection with its own ID. That's not really a big deal for other types since they're inline anyway - but since this needs to be outlined it creates possibly two ids where there only needs to be one or zero.
One variant would be to encode this in the row type. Another variant would be something like what we do for React Elements where they're arrays but tagged with a symbol. For simplicity I stick with the simple outlining for now.