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

fix: use Record for properties field in Schema Object #135

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vaibhavrajsingh2001
Copy link

@vaibhavrajsingh2001 vaibhavrajsingh2001 commented May 7, 2024

Using mapped types like { [propertyName: string]: SchemaObject | ReferenceObject } doesn't ensure the key to be of type string. Instead, TypeScript interprets the key as string | number.

image

This behaviour can be verified in the following playground

Due to this issue, if you iterate over the properties of a schema, TypeScript can't be sure that the key is of type string. Instead the key will be interpreted as of type string | number.

Using a Record allows to strictly type the key to only be of type string, which can be verified here

image

It's a known TypeScript issue microsoft/TypeScript#48269

@vaibhavrajsingh2001
Copy link
Author

Hey @pjmolina, came across this issue while I was trying to iterate over the properties field of a SchemaObject.
Would appreciate a review here 🙏.

@@ -292,7 +292,7 @@ export interface SchemaObject extends ISpecificationExtension {
anyOf?: (SchemaObject | ReferenceObject)[];
not?: SchemaObject | ReferenceObject;
items?: SchemaObject | ReferenceObject;
properties?: { [propertyName: string]: SchemaObject | ReferenceObject };
properties?: Record<string, SchemaObject | ReferenceObject>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest : Record<string, SchemaObject | ReferenceObject | undefined>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the value of any field in properties can ever be undefined.

In YAML spec files, If no value is assigned to a field in properties, Swagger automatically treats the field as of type any.

image

However, in JSON format, it's not allowed to assign an undefined value to a filed in properties

image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the worst case, it'll be an empty object, which again is a valid SchemaObject

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remidewitte do you think undefined would be needed here?

Copy link
Contributor

@RobinTail RobinTail May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Properties assigned with undefined are not exported to YAML and JSON at all.

console.log(
  yaml.stringify({
    property: undefined,
  }),
);

console.log(
  JSON.stringify({
    property: undefined,
  }),
);
{}

{}

Therefore, the only benefit for adding undefined is for conditional properties:

yaml.stringify({
  property: isIncluded() ? "something" : undefined
});

But that condition can be implemented differently, without involving undefined at all:

yaml.stringify({
  ...(isIncluded() ? {property: "something"} : {})
});

Copy link
Contributor

@RobinTail RobinTail May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK excellent.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, should I update the types to Partial<Record<string, SchemaObject | ReferenceObject>>?

Copy link
Contributor

@RobinTail RobinTail May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaibhavrajsingh2001 , Taking into account the concept and the goal of this package:

TypeScript library to help building OpenAPI 3.x compliant API contracts.

I'd say: there are more benefits in Partial<> than any harm.

Under the hood Partial<> makes all properties optional ?:, while does not explicitly welcome undefined,
which can be assigned, but still there is no harm of that — it will not go further (JSON and YAML stringifiers remove it). It's helpful both for conditional writing and for avoiding runtime errors while reading, like in the @remidewitte 's case

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR @RobinTail @remidewitte

@pjmolina
Copy link
Contributor

pjmolina commented May 23, 2024

I have contradicting feelings here:

  • We cannot fix TS or JS here (clearly out of scope)
  • Having a string key in dictionary does not prevent for accessing the keys by index (JS semantics) nor we should forbid it.
  • If we fix this one, we should be coherent and fix all dictionary-like access we use.

To be honest, not sure if it is makes worthy. And if we are breaking uses cases for anyone else.

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 this pull request may close these issues.

4 participants