Skip to content

Handle naming collisions #116

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

Closed
masaeedu opened this issue Sep 29, 2017 · 5 comments
Closed

Handle naming collisions #116

masaeedu opened this issue Sep 29, 2017 · 5 comments
Assignees
Labels

Comments

@masaeedu
Copy link

masaeedu commented Sep 29, 2017

Currently the converter dumps primitive types (created due to nillability, unions, arrays, etc.) into a list of export type statements at the top of the generated definition file.

The problem is that none of the types are namespaced, so when running json2ts on a large JSON schema file with lots of type definitions, common properties like Username will end up creating lots of duplicate export type Username = (string | null); declarations.

These type declarations should either be namespaced to avoid conflict, or inlined into the interface declarations.

Test it out with this schema to see it break.

@bcherny
Copy link
Owner

bcherny commented Sep 30, 2017

Good catch @masaeedu.

A couple of fixes come to mind:

  1. Apply a collision avoidance algorithm to append a counter to names that are already used (eg. Foo, Foo1, Foo2, etc.)
  2. Namespace all types referenced from a schema

(2) has the advantage of not mutating the original type's name, and its namespacing has the advantage of making it clear where a type came from (though to be fair, "find usages" does that too). However, it is hard to implement because json-schema-ref-parser inlines all external schemas for us, so we don't actually know if a given branch of a JSONSchema tree came from an external source, or if it was part of the original file. It would be pretty simple to add a flag to externally resolved branches, but to do that we'd need to either fork json-schema-ref-parser (not a good idea) or contribute a patch to it (hopefully others share our use case). Additionally, (2) would be a breaking change for json-schema-to-typescript users.

The thing is, even with namespacing, collisions are still possible (eg. what if two top-level named schemas share the id Foo?). So (1) seems to be the more generic solution, because it covers this case too. It is also cheaper, because it avoids the need to fork/update json-schema-ref-parser.

What do you think @masaeedu? If we went with (1), would that work for you?

@bcherny bcherny self-assigned this Sep 30, 2017
@bcherny bcherny added the bug label Sep 30, 2017
@masaeedu
Copy link
Author

masaeedu commented Oct 1, 2017

@bcherny Thanks for looking into this. My ideal solution would be to just inline all of these types and not have them at all. Is this not viable?

@masaeedu
Copy link
Author

masaeedu commented Oct 1, 2017

Approach (1) would be fine as a quick fix I suppose, it's just a little confusing to have these numbered types without the consumer understanding what the numbers mean. Re: namespacing conflicts; if you namespace using every type down from the root, wouldn't that eliminate all conflicts unless there's duplicate definitions in the schema?

@bcherny
Copy link
Owner

bcherny commented Oct 1, 2017

My ideal solution would be to just inline all of these types and not have them at all.

That would be a feature on top of the fix for the legitimate bug you found. I've created an issue for it: #117.

it's just a little confusing to have these numbered types without the consumer understanding what the numbers mean

I agree completely.

Re: namespacing conflicts; if you namespace using every type down from the root, wouldn't that eliminate all conflicts unless there's duplicate definitions in the schema?

Two or more JSON-References might point to the same JSON-Schema. Depending on the order in which the root JSON-Schema is traversed, the type will be given a different namespace. For example if schema A references schema C, but schema B also references C, then C might compile to A.C or B.C depending on the order in which we traverse the schemas. We could fix this by adding an alias A.C = B.C.

If we go with (1), then this example will compile to A, B, and C. If A and B reference different schemas that both happen to have the name C, then with (1) that will be compiled into A, B, C, and C1.

The thing to keep in mind is if A references two schemas called C (possible if the two Cs are, for example, externally maintained schemas), we still need a way to avoid naming collisions (or we can throw an error, but this is annoying because if the schemas are hosted elsewhere the user may not be able to modify them).

I'll go with (1), and then work on #117 (or better yet, try to do it yourself and PR the feature!). In the future, namespacing will improve case B's output (see below) - I'm tracking that separately in #119.


For posterity, here are the cases we need to handle:

A. A -> C, B -> C (schemas A and B both reference the schema C)

  • (1) would give top level schemas A, B, and C
  • (2) would give top level schemas A and B, the namespaced schema A.C, and an alias B.C = A.C

B. A -> C, B -> C' (schema A references a schema C, and schema B references a different schema C that happens to have the same name)

  • (1) would give top level schemas A, B, C, and C1
  • (2) would give top level schemas A and B, and the namespaced schemas A.C and B.C

C. A -> C, A -> C' (schema A references two different schemas that both have the name C)

  • (1) would give top level schemas A, C, and C1
  • (2) would give the top level schema A, and the namespaced schemas A.C and A.C1

@bcherny bcherny changed the title Primitive types should be namespaced Handle naming collisions Oct 1, 2017
@bcherny bcherny closed this as completed in dd00edd Oct 3, 2017
@bcherny
Copy link
Owner

bcherny commented Oct 3, 2017

+ [email protected]

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

No branches or pull requests

2 participants