tree: Allow array from map and map from array #22036
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: cb30d7f The changes in this PR will be included in the next version bump. This PR includes changesets to release 157 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
area: dds
Josmithr
reviewed
Jul 26, 2024
Co-authored-by: Joshua Smithrud <[email protected]>
noencke
reviewed
Jul 26, 2024
| it should work, even if those iterables are themselves arrays or maps. | ||
| To avoid this being a breaking change, a priority systems was introduced. | ||
| ArrayNodes will only be implicitly constructable from JavaScript Map objects in contexts where no MapNodes are allowed. | ||
| Similarly MapNodes will only be implicitly constructable from JavaScript Array objects in contexts where no ArrayNodes are allowed. |
There was a problem hiding this comment.
Can we improve upon this even more in the future? You could have e.g. Map<string, string> | Array<string>, and we would be able to tell the difference between the two for an input like [string] (array) or [[string, string]] (map), so long as the array isn't empty. Or, is that too subtle of an edge case that it will confuse users, and it's better to simply keep the dividing line as it is? I'm inclined to think that it's too subtle, but I'm curious what you think.
There was a problem hiding this comment.
Yes. You can't tell as [string, string][] from a number[] in the case where they are empty, so in general we can't do that general improvement always, but we can do some cases of it (ex: all non-empty cases) in the future, and doing so will be a non-breaking change since it would convert cases which were ambiguous to working, and cases which had type errors on the children to type errors on the parent. Both those changes will be allowed.
There was a problem hiding this comment.
Note that the kind of change you mention (being more picky, causing error higher up the tree and less ambiguity) is generally safe, where the change I made here (allowing more options) is prone to causing new ambiguity issues which is what needed the extra care and priority thing
There was a problem hiding this comment.
Good observation, that makes sense. Thank you.
| /** | ||
| * Indicates a compatibility level for inferring a schema to apply to insertable data. | ||
| * @remarks | ||
| * Only the highest compatibility options are used. |
There was a problem hiding this comment.
Suggested change
| * Only the highest compatibility options are used. | |
| * Only the highest quality compatibility options are used. Here at `getPossibleTypes` we pride ourselves on bringing the freshest options direct to you for a fraction of the cogs of our competitors. We stand by our enums - if you are dissatisfied* in any way, your `allowedTypes` and `data` will be returned to you free of charge and unmutated. [* Satisfaction only guaranteed after TS 4.9](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-9.html#the-satisfies-operator) |
There was a problem hiding this comment.
I think your explanation is inaccurate. The quality here is relative, so when low is the highest quality available, we still use it, which could invalidate such a guarantee.
| /** | ||
| * Not compatible. Constructor typing indicates incompatibility. | ||
| */ | ||
| None = 0, |
There was a problem hiding this comment.
The "lower compatibility levels" that might be added in the future would be "between" None and Low, right? Should we adjust the numeric enum values to reflect that, so there is room to add the future values? E.g.
None = 0, Lower = 253, Low = 254, Normal = 255
Or maybe we can use 0.5 for Lower, lol.
There was a problem hiding this comment.
We can always change these values in the future (this is not a public const enum: its neither public nor const), not to mention there are a lot of numbers between 0 and 1.
Co-authored-by: Noah Encke <[email protected]>
noencke
approved these changes
Jul 26, 2024
⯅ @fluid-example/bundle-size-tests: +1023 Bytes
Baseline commit: 5797dc1 |
…to fromIterable
alexvy86
approved these changes
Jul 26, 2024
This was referenced Aug 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Allow constructing ArrayNodes from Maps and MapNodes from arrays when unambiguous.
Also removes some no longer needed shallow copies in the map and array constructors .
See changeset for details.
Reviewer Guidance
The review process is outlined on this wiki page.