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

Add a JSON schema #147

Closed
wants to merge 4 commits into from
Closed

Add a JSON schema #147

wants to merge 4 commits into from

Conversation

UnderKoen
Copy link

Description

I would like to have the benefits of #130 without using js.

Justification (Why)

#146

How Can This Be Tested?

  • Create a JSON file
  • Define field $schema with value https://raw.githubusercontent.com/UnderKoen/syncpack/main/schema.json
  • Make a valid / invalid config and test
  • Alternatively you can use https://www.jsonschemavalidator.net/ for testing, here you need to paste the content of schema.json

The new types are the same as before due to all options having one field.

For CustomTypeConfig the type is not an union anymore, but from what I could tell it wasn't needed to be a union.
Copy link
Owner

@JamieMason JamieMason left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this @UnderKoen, I can pick up most of this from here but I did leave some questions about things I didn't understand if you could help me out please.

@@ -39,7 +38,7 @@ export namespace SemverGroupConfig {
range: SemverRange;
}

export type Any = Union.Strict<Ignored | WithRange>;
export type Any = GroupConfig & Partial<Ignored & WithRange>;
Copy link
Owner

Choose a reason for hiding this comment

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

What was happening when you made this change? Maybe I can help. This doesn't seem right as Ignored and WithRange are mutually exclusive but this change makes them a combination of both types added together.

This comment also apples to the same change made below.

Copy link
Author

@UnderKoen UnderKoen Jul 7, 2023

Choose a reason for hiding this comment

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

What was happening when you made this change?

ts-json-schema-generator doesn't understand the union type.

This doesn't seem right as Ignored and WithRange are mutually exclusive but this change makes them a combination of both types added together.

Maybe I don't understand the union correctly. It is a combination of the types right?
The Union.Strict makes sure when type is Ignored all other fields of Ignored are also required.

What you want would be:

Suggested change
export type Any = GroupConfig & Partial<Ignored & WithRange>;
export type Any = Ignored | WithRange;

Here it is not possible to define Ignored and WithRange

Copy link
Owner

Choose a reason for hiding this comment

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

Hopefully this makes sense, what Union.Strict<Ignored | WithRange> is describing is:

// Accept exactly one or the other of the types inside <here>
// - do not allow an Ignored with additional properties
// - do not allow a WithRange with additional properties
// - do not allow an Ignored with a WithRange on top of it
Union.Strict<
  // Accept either one or the other, but not both at the same time
  Ignored | WithRange
>

In contrast Ignored & WithRange is creating one new type, which is Ignored with all the properties of WithRange merged together on top into one object.

Maybe give it a try by going back to how it was but remove every usage of Union.Strict and see if everything still works. It may be fine, but I think possibly there may be some cases where the problems linked here may come up.

export type Any = Union.Strict<
NameAndVersionProps | NamedVersionString | UnnamedVersionString | VersionsByName
>;
export type Any = NameAndVersionProps | NamedVersionString | UnnamedVersionString | VersionsByName;
Copy link
Owner

Choose a reason for hiding this comment

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

What was happening when Union.Strict was still there please?

Copy link
Author

Choose a reason for hiding this comment

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

Union.Strict was doing nothing here, because all of these types have the same fields.

JamieMason added a commit that referenced this pull request Aug 13, 2023
@JamieMason
Copy link
Owner

Released in 11.2.1

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.

2 participants