Skip to content
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

Type errors against Google's recommended Schema JSON-LD's #130

Closed
BenRacicot opened this issue Oct 25, 2020 · 5 comments · Fixed by #163
Closed

Type errors against Google's recommended Schema JSON-LD's #130

BenRacicot opened this issue Oct 25, 2020 · 5 comments · Fixed by #163

Comments

@BenRacicot
Copy link

Firstly, thank you for this lib and your maintenance of it.

After setting up the typings and using them against Google's recommended Schema markup. In this case I'm seeing errors against Product and it's nested aggregateRating can you help me understand how to use these typings with Google's recommended versions?

Error:

(property) "aggregateRating"?: SchemaValue<IdReference | AggregateRatingLeaf | EmployerAggregateRatingLeaf>
The overall rating, based on a collection of reviews or ratings, of the item.

Type '{ "@type": "AggregateRating"; ratingValue: string; reviewCount: string; }' is not assignable to type 'SchemaValue<IdReference | AggregateRatingLeaf | EmployerAggregateRatingLeaf>'.
  Types of property '"reviewCount"' are incompatible.
    Type 'string' is not assignable to type 'SchemaValue<number>'.ts(2322)
schema.d.ts(8042, 5): The expected type comes from property 'aggregateRating' which is declared here on type 'Product'
@Eyas
Copy link
Collaborator

Eyas commented Oct 25, 2020

:( Yeah. https://schema.org/reviewCount says it's an schema.org/Integer, but all the examples also use strings.

In reality, every Schema.org property is implicitly convertible from a string. But making everything a string can also defeat the purpose of type checking. So right now I've approached a per-type opt-in strategy. Opting in Integer feels... too much? Some discussion previously in #37.

HOWEVER, https://devblogs.microsoft.com/typescript/announcing-typescript-4-1-beta/#template-literal-types

I wonder if we can require TypeScript 4.1 in the next major release (0.8.x) and define Integer to include any numeric type? Except microsoft/TypeScript#40580 implies this is kind of limited.

Open to feedback from the community here.

@BenRacicot
Copy link
Author

Thanks for your reply @Eyas and great insight. I can sympathise with the direction you've taken.

I also understand that specification (schema.org) does not imply implementation (Google). That being said, these typings must allow deviation from the official specification in order to appease the major search engines, even if they were to completely become separate.

Regarding your idea
I think string or any integer type is probably going to be required for me to use the typings and is still helpful in terms of type checking (better than any?)

@Eyas
Copy link
Collaborator

Eyas commented Oct 25, 2020

The other thing going on is that schema.org's own examples use strings for these properties.

Right now, I'm not as keen to support string on every Integer, but one idea that makes sense is to have an opt-in list to properties that are commonly treated as strings. And locally add | string just to these properties.

If we went with such an implementation, what properties would need to be in on list to meet an ergonomic use? I assume:

  • reviewCount
  • ratingCount

Let me know if you run into anything else

@Eyas
Copy link
Collaborator

Eyas commented Jul 31, 2021

Not quite Integer, but since TypeScript 4.1.5 we can actually express:

type NumericStirng = `${number}`;
const allowed_1: NumericStirng = '012420932892389.43';
const allowed_2: NumericStirng = '43893';
const not_allowed_1: NumericStirng = '012.4.4';
const not_allowed_2: NumericStirng = '';
const not_allowed_3: NumericStirng = 'abc';

This is more appropriate for the Number type. But it's probably a step forward to also include it in Integer. The only note about using the `${number}` type for Integer is that ... if TypeScript ever has a proper Integer string type (or a way to express Exclude<`${number}`, `${string}.${string}`> which doesn't work today), we'll introduce a "breaking" change to disallow non-integral numeric strings for Integer. But that seems fair game.

Eyas added a commit to Eyas/schema-dts that referenced this issue Jul 31, 2021
Schema.org Numbers, Integers, and Floats support being assigned to
strings with the appropriate fromat (e.g. "4394" for Integer). With
TypeScript 4.1.0 and beyond, this can be expressed with the
`` `${number}` `` type. This pushes the minimum required version for
schema-dts from 3.4.0 to 4.1.0, and is thus a breaking change.

Note there's a future situation where we would RESTRICT the `Integer`
type further; so that only valid integral numbers are assignable to it.
This will be another breaking change for another time, when TypeScript
can finally represent this.

Fixes google#130.
@Eyas Eyas closed this as completed in #163 Jul 31, 2021
@Eyas
Copy link
Collaborator

Eyas commented Aug 2, 2021

Fixed in v0.10.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants