feat(tree): getOrInsert mapNode alpha API - #27787
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (135 lines, 5 files), I've queued these reviewers:
How this works
|
| } | ||
| this.set(key, fallbackValue); | ||
| return this.get(key); | ||
| return fallbackValue as TreeNodeFromImplicitAllowedTypes<T>; |
There was a problem hiding this comment.
I'm concerned the cast may hide some cases where we can't actually rely on the fallbackValue being the same thing as what this.get(key) might return. @CraigMacomber - is there a safe way to avoid having to call this.get(key) here?
There was a problem hiding this comment.
It wasn't letting the return happen without the cast unfortunately so curious about this as well
There was a problem hiding this comment.
This code is incorrect, and the typing is right about your value being wrong.
fallbackValue is an insertable value, which might not be a node, but might implicitly get constructed into one.
You have 4 choices for how to handle this:
- force the insertable value into a node before inserting (Like TreeBeta.create can do), then insert that and return it
- let
setdo the implicit construction, then look it up withget - Require input to be a node/tree value and not an insertable value
- Remove the return value
2 is the simplest, so I'd recommend it, but 1 is also reasonable.
There was a problem hiding this comment.
https://fluidframework.com/docs/api/fluid-framework/treenode-class has some details about implicit node construction.
There was a problem hiding this comment.
Thanks Craig.
@brrichards - Please have a look at option 1, but if it seems too cumbersome then I'd be ok with option 2.
There was a problem hiding this comment.
updated with option 1 following sort of what set does, but if this isnt ideal can change to option 2.
| * there is no guarantee that the entry for the given key will be the current one (if any) or the fallback one. | ||
| * If such a guarantee is important, then consider using constraints to ensure the edit only applies when appropriate. | ||
| * | ||
| * This API is not equivalent to the following alternative: |
There was a problem hiding this comment.
Nit
| * This API is not equivalent to the following alternative: | |
| * This API is **not** equivalent to the following alternative: |
Josmithr
left a comment
There was a problem hiding this comment.
Left a couple of docs suggestions. But overall they look great! Thanks for being so thorough!
|
wait for #27824 to merge |
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Bundle size comparisonBase commit: No CI build was found for the PR HEAD commit. This shouldn't happen — the workflow only runs after the PR's |
Description
Adds getOrInsert alpha API for mapNode. Updates current mapNode alpha API changeset
Reviewer Guidance
The review process is outlined on this wiki page.