Add ts enums

[jcogilvie]

I added a few things here (with tests):

1. ability to reference other .json files (locally) with $ref, and choose whether to have the referenced types declared with the existing declareReferenced option.
2. option to export the generated interfaces
3. fall back to filename for naming something without ID or title
4. option to generate ts enums (int-backed instead of string union) and additionally a proprietary schema extension tsEnumNames so that you can specify which integers to use, if you care, like so:

enum: [1, 2, 3]
tsEnumNames: ["one", "two", "three"]

This will generate

enum Whatever {
    one=1,
    two=2,
    three=3
}

Additionally any nested enums will be extracted into top-level types.

[jcogilvie]

This is the only real concern I have about the code here. I'll call attention to the example of this on the enum test case.

[bcherny]

Thanks for the contribution - will take a look this week.

[bcherny]

Don't check this in, I don't use this setup.

[jcogilvie]

This setup doesn't use anything environment-specific. It's generalizable so long as we use mocha, so if someone writes TS in vscode, this run configuration can save them the work of figuring out how to debug tests. I think it adds value, but obviously it's your project so I'll remove it if you still think I should.

[bcherny]

I see what you mean. Also I realized that .vscode/settings is already checked in - let's keep this guy here :)

[bcherny]

@jcogilvie Again, thanks for the contribution. At a high level, I have a few concerns:

1. ability to reference other .json files (locally) with $ref, and choose whether to have the referenced types declared with the existing declareReferenced option.

This is useful, but your implementation does not seem to comply with the official spec. See https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/develop/tests/draft4/ref.json

2. option to export the generated interfaces

As commented before, this is a great idea, and should be enabled by default. No need to add the ability to disable export unless you have a compelling use case.

3. fall back to filename for naming something without ID or title

Also a good idea, but see 1.

4. option to generate ts enums (int-backed instead of string union) and additionally a proprietary schema extension tsEnumNames so that you can specify which integers to use.

Another good idea, but the implementation is overly permissive. Again, see the comments.

If you want, it could be helpful to split 1+3, 2, and 4 into separate PRs.

[bcherny]

Also please rebase off master and run the linter (gulp lint).

[jcogilvie]

How do you feel about marking the generated enum util classes as beta or something, maybe by renaming the setting?

I definitely need them as-is for my use case, but I understand that you guys want to discuss them further. I also don't want to spend much longer chasing merges on this PR. It's kind of big.

[jcogilvie]

Re: $ref implementation, I haven't implemented the full spec for sure, but the spec does call out support for file:// references which in my mind means having support for relative paths on the filesystem is valid, which is all that my implementation does.

[bcherny]

Hey @jcogilvie, think you can jump on gitter? https://gitter.im/json-schema-to-typescript/Lobby?utm_source=share-link&utm_medium=link&utm_campaign=share-link

[jcogilvie]

To sum up yesterday's conversation, I agree that string enums should be represented as string unions. Integer enums should use my new code.

If I make that change and kill the util code, will the rest of the PR be accepted as-is?

[bcherny]

Is an enum of ['', 'foo'] valid? Should the check be const hasValue = enumValues.length > 0?

[jcogilvie]

I don't know what what would resolve to, symbolically, when trying to reference the member?

These are only used in the case of integer backed enums, and I can't think of why we would assign ''=0.

[bcherny]

An enum value of 0 is valid, no?

[jcogilvie]

Yeah but I'm not sure a symbolic value of "empty string" is something that we want to produce in an enum declaration. It seems ripe for conflation with null and undefined semantics.

[bcherny]

That's fine, but this package should support what TS supports. Ts supports enum a {''}, so should we.

If you want to avoid empty string enums, I recommend a tslint rule.

[jcogilvie]

We don't generate string identifiers. We generate literal identifiers. So this would be a very special case for a not very often used item, and I'm not sure how we would differentiate it... Our output is:

export enum Bar {
  One = 1,
  Two = 2,
  Three = 3
}

Not

export enum Bar {
  "One" = 1,
  "Two" = 2,
  "Three" = 3
}

Really we need to be worried about what the schema supports--and we support strings or integers as enum member values. If they added '' as a value in a string enum, it would work fine (as this EnumValue class is used for int enums only). But as far as our own proprietary extension for the names goes, we can declare whatever requirements we want on those values. An empty string is a special case here (as we don't support any other quoted literal); do you think it's worth the effort?

Although I suppose if you had "''" (a double-quoted set of single-quotes) as the tsEnumName in the schema, that might print an empty string as the identifier...

[bcherny]

Let's throw an error here, and treat it as tech debt?

[jcogilvie]

Look at the call site for constructing these. This is a zipped array; assuming we pass all other validation for an int enum, what are the possible outcomes here that we need to handle?

I actually think most of this can probably go away... this was used to propagate string values in enum: [...] up to identifiers for enum declaration, which we no longer do.

var enumValues = zip(rule.tsEnumNames || [],
                // If we try to create a literal from an object, bad stuff can happen... so we have to toString it
                rule.enum!.map(_ => new TsType.Literal(_).toType(this.settings).toString()))
              .map(_ => new TsType.EnumValue(_))

[bcherny]

SGTM

[bcherny]

If I make that change and kill the util code, will the rest of the PR be accepted as-is?

I made a last round of comments - it's all minor stuff. The PR looks great @jcogilvie. I look forward to finally getting it merged! In the future, breaking code up into separate PRs by feature could be a good idea :)

[jcogilvie]

Ha. Yes, this PR grew since I first opened it. My own use case became clearer and I had to add more stuff. In the future I'll probably keep one mainline branch to house all my aggregate changes with individual branches for each.

[bcherny]

@jcogilvie Ping me here when code is ready to go and I'll merge it.

[jcogilvie]

@bcherny I think we're good to go.

[bcherny]

Looks good - thanks @jcogilvie!