-
Notifications
You must be signed in to change notification settings - Fork 535
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 more cases in Tree.is #20973
Allow more cases in Tree.is #20973
Conversation
⯅ @fluid-example/bundle-size-tests: +182 Bytes
Baseline commit: 5a15835 |
if (actualSchema === undefined) { | ||
return false; | ||
} | ||
if (isReadonlyArray<LazyItem<TreeNodeSchema>>(schema)) { |
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.
Do you want to use normalizeAllowedTypes
here to avoid manually handling the array and lazies?
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 started with that approach, but it incurs a lot more overhead in both the single item case and the array case. This avoids allocating the set, gets an early return on a match, avoids boxing single items into an array, avoids a bunch of calls, and extra loop etc. The code for this approach is only slightly larger.
Normalize is mainly used in places where we can cache the result and reuse it, which is not the case here (unless we wend with a different API, and returned a node filter predicate).
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.
Got it, sounds good. I wonder if we could get the benefits of the early return and no Set() allocation by using a generator that returns an iterator of the normalized schema. Something to try, later, if we find ourselves doing this same kind of thing in multiple places.
Description
This makes 3 changes to Tree.is:
Interestingly
#2
above is the only case that couldn't be covered byinstanceof
(assuming TypeScript 5.3): we could makeinstanceof
do all narrowing currently done with Tree.is, except for this new case. The presence of this case thus seems to motivate keepingTree.is
if for no reason other than it can support this additional pattern whichinstancof
cannot.Reviewer Guidance
The review process is outlined on this wiki page.
This is a public API change, and thus needs extra review on the API and how it will impact customers.