nest
nest copied to clipboard
nestjs
fix(core): fix type and return value of `Reflector#getAllAndMerge`
When only a single metadata value is present in the list of targets for Reflector#getAllAndMerge, that value gets wrapped in an array if it is not already an array or an object.
Previously, single values were returned as is, so the method could actually return any value. Now the method always returns an array or an object, and the type constraint for TResult is changed to reflect that.
Fixes #12231
PR Checklist
Please check if your PR fulfills the following requirements:
- [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
PR Type
What kind of change does this PR introduce?
- [x] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, local variables)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] CI related changes
- [ ] Other... Please describe:
What is the current behavior?
getAllAndMerge can return any value when only one of its targets has a metadata value set, contradicting its declared return type.
Additionally, its return type declaration contradicts its actual and documented behavior because it claims to always return an array.
Issue Number: #12231
What is the new behavior?
When only a single non-undefined value is present in the requested metadata of the targets, that value is now wrapped in an array. This matches the existing behavior when no non-undefined value is present (returning an empty array) or more than one primitve value is present (creating an array with these values).
The type constraint for TResult has been changed to any[] | object to reflect the fact that the method can also return a (merged) object. This allows the user to specify a specific object (non-array) type as the type parameter if they know that the metadata is always set to that type.
I decided not to change the default value for TResult (leaving it as any[]) to avoid a breaking change. Changing the default value would break most uses of the method without an explicit value for TResult.
Does this PR introduce a breaking change?
- [ ] Yes
- [x] No
Other information
A separate PR changing the default value of TResult along with some clarification in the documentation could be added later.
Pull Request Test Coverage Report for Build 6a015382-2d3a-496a-acbb-092b784945ae
- 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+0.005%) to 92.518%
| Totals | |
|---|---|
| Change from base Build 10e490cf-efc2-421e-837e-12149683818e: | 0.005% |
| Covered Lines: | 6430 |
| Relevant Lines: | 6950 |
💛 - Coveralls
I rebased my changes and integrated them with the changes introduced by #12237. That change is definitely a great feature and makes using custom metadata much easier and type safe.
The return type of getAll when using a ReflectableDecorator was wrong in that PR, the two sides of the conditional type were simply swapped around. I included a fix in my PR.
The return type of getAllAndMerge was also wrong and didn't reflect the object merging and array wrapping behavior of the function (the same as my original issue).
Fixing this leads to the slightly weird but correct type T | [] if T is a not an array but an object. This reflects the fact that the function returns an empty array if the metadata doesn't exist or is undefined for all targets, but will return the merged object if at least one metadata object is present. That type is hard to work with, since you either have to check for an empty array using Array.isArray or just remove it from the type using as T, making the assumption that at least one target has metadata assigned. A more natural return value for the object merging case would be undefined, but there is no way to tell the object and array merging cases apart at runtime because that information is only contained in the types.
The only ways I can think of to fix this would be to either introduce a separate method for the object merging case which returns undefined instead of [], or to add more metadata to ReflectableDecorator (which would require a manually set option to opt-in to the object merging case). Both seem clunky. Maybe someone has a better idea.
As an aside, there is currently no way to test the types with something like expect-types. I see that the other PR introduced some @ts-expect-error comments, but those don't actually check the error message text. They don't let you make positive assertions about the type of a variable, only that x is not assignable to y, which isn't very helpful. I included the expected types of return values in my new tests, but they are not checked.