feat: improve metadata value types #194
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
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
davidlj95
force-pushed
the
stacked/feat-improve-metadata-value-types
branch
2 times, most recently
from
80fd0da
to
5f35752
Compare
📦 Bundle size (Angular v15)Git ref: 8508c47
|
📦 Bundle size (Angular v16)Git ref: 8508c47
Base size data is not available yet. Try again when the CI/CD has finished running on main branch |
📦 Bundle size (Angular v17)Git ref: 8508c47
|
davidlj95
force-pushed
the
stacked/feat-improve-metadata-value-types
branch
from
5f35752
to
8508c47
Compare
davidlj95
commented
Jan 18, 2024
Comment on lines
-1
to
-3
| export interface MetadataValues { | ||
| [key: string]: unknown | ||
| } |
There was a problem hiding this comment.
This was giving issues, as it's a specific type of object. An object that implements an interface without index signature wouldn't comply with this. Despite yet being valid metadata values. So going for a broader definition
davidlj95
commented
Jan 18, 2024
|
|
||
| export type MetadataRouteStrategy = <T>( |
There was a problem hiding this comment.
Doesn't make sense to specify type here, given the strategy should load some object from route, without caring what the object contains. It's not its responsibility
|
🎉 This PR is included in version 1.0.0-alpha.31 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.
In the metadata set by route + service E2E test, the code to use the
MetadataServicewas a bit awkward in the types sense. We had to cast tounknownand then toMetadataValues. Not a great DXRethinking the types referring to metadata values and their usages to provide as good DX as we can
Changes:
MetadataValuesbe justobject. Which is anything other than a primitive:This allows metadata values to be whatever JSON object unless you specifically opt-in for some types. There's no specific JSON type in Typescript So that's the best option AFAIK. Not using
unknownas that includes primitives and we're sure we want an object. Indeedunknownallowsundefinedtoo. Not usinganyas it's a bad practice + same issues asunknownIn. Consumers can declare data to put as argument in a separate variable or useMetadataService, make thesetmethod generic to allow to opt-in to type-safe values argumentsatisfiesMetadataRouteStrategytype returnMetadataValuestooMetadataRouteDatageneric to allow specifying type of route data. Defaulting to broadMetadataValuesakaobjectin case you don't want to opt-in for safe types.This allows to:
XMetadata&Xtypes to declare type safe metadata values. LikeOpenGraphMetadata&OpenGraph.MetadataRouteData<OpenGraphMetadata & TwitterCardMetadata>to ensure route data is properly typed. Either when declaring a var type or usingsatisfies.MetadataValues/object, so it's open for extension but closed for modification. Consumers can specify a stricter type if they want to ensure types are ok or the IDE to help them write them.This doesn't allow to:
OpenGraph.typeorTwitterCard.card) don't match. Because the JSON types those fields asstring. You can't useas constin there. Yet. import ConstJson from './config.json' as const; microsoft/TypeScript#32063