Export additionalProperties as intersection for complex types #383

fabrykowski · 2021-04-21T14:00:37Z

A quick fix for #299, #356 and #402.

Given the schema

{
  "title": "AdditionalProperties",
  "type": "object",
  "properties": {
    "foo": {
      "type": "string"
    }
  },
  "additionalProperties": {
    "type": "number"
  }
}

it is rendered by the library as

export interface AdditionalProperties {
  foo?: string;
  [k: string]: number;
}

which results in the compiler error

TS2411: Property 'foo' of type 'string | undefined' is not assignable to string index type 'number'.

This PR renders the schema above as

export type AdditionalProperties = {
  foo?: string;
} & {
  [k: string]: number;
};

which is valid TypeScript.

fabrykowski · 2021-04-21T14:01:12Z

test/__snapshots__/test/test.ts.md

@@ -5682,6 +5691,142 @@ Generated by [AVA](https://avajs.dev).
    }␊
    `

+## realWorld.jsonschema.js


I have no idea, why this section is moved

bohjak · 2021-05-06T09:12:22Z

But it doesn't seem to actually fix the problem? Or am I doing something wrong?

The generated type is indeed valid TypeScript in the sense that it does compile. But if you try to create an object with it, it will still fail:

export type AdditionalProperties = {
  foo?: string;
} & {
  [k: string]: number;
};

const bar: AdditionalProperties = {
    foo: 'baz'
}

/*
Type '{ foo: string; }' is not assignable to type 'AdditionalProperties'.
  Type '{ foo: string; }' is not assignable to type '{ [k: string]: number; }'.
    Property 'foo' is incompatible with index signature.
      Type 'string' is not assignable to type 'number'.
*/

TypeScript Playground

fabrykowski · 2021-05-06T13:50:38Z

It is impossible to create such an object in TypeScript, but you can use it after a cast or assertion.

export type AdditionalProperties = {
  foo?: string;
} & {
  [k: string]: number | undefined;
};

const bar = {
    foo: 'bar',
    baz: 42
} as unknown as AdditionalProperties;

const foo = bar.foo; // type: string | undefined
const baz = bar.baz; // type: number | undefined
const tmp = bar.tmp; // type: number | undefined

TypeScript Playground

fabrykowski · 2021-05-15T16:12:16Z

This is especially of interest, when a received JSON object – such as an HTTP request body – is parsed.

fabrykowski · 2021-07-01T09:12:31Z

We need this in our code base – any chance of a merge?

Michael-Hering · 2021-07-21T21:19:37Z

+1 - TS serverless files are a huge win for teams building serverless apps in TS and this PR would allow that to happen

maximseshuk · 2021-08-11T22:07:36Z

+1

luc-heyday · 2021-08-12T12:20:12Z

+1

electblake · 2021-08-20T17:02:09Z

+1

fabrykowski · 2021-10-26T20:53:04Z

Any news? We could really use this in our production code.
@bcherny This PR could close three issues at once.

# Conflicts: # test/__snapshots__/test/test.ts.md # test/__snapshots__/test/test.ts.snap

macsj200 · 2021-11-09T21:19:26Z

I would greatly appreciate this fix as I'm currently blocked by serverless/typescript#27.

fabrykowski · 2022-05-12T07:23:44Z

@bcherny – this PR would close multiple tickets. Would it be possible to initiate a review?

G-Rath · 2022-05-12T08:02:47Z

@fabrykowski (and potential reviewers) please keep in mind my comment here: #356 (comment)

This change will be breaking as it prevents you from using declaration merging (which can only be done with interfaces) which can be a very powerful tool with additionalProperties.

A better solution would be to generate a KnownProperties interface that is used in-place of the anonymous interface in the union that this change has generated.

e.g.

interface AdditionalPropertiesKnownProperties {
  foo?: string;
}

export type AdditionalProperties = AdditionalPropertiesKnownProperties & { 
  [k: string]: number;
};

fabrykowski · 2022-07-31T20:07:32Z

@G-Rath – in the example you gave only the finished type AdditionalProperties is exported – which means that declaration merging would not be available. On the other hand exporting the [...]KnownProperties-interface for each type with additionalProperties seems a tad unwieldy and pollutes – in my opinion – the namespace.

I would argue that declaration merging is a very useful tool for DefinitelyTyped, allowing for example to define specific headers; should not the aim of this package be the opposite? If I have a JSON-schema and I validate a JSON-object (e.g. via ajv) then I can safely cast the object to (or assert it to be) the generated TypeScript-type! (This is, at least, the way I use this package.)

If there is strong demand for it, I could add another optional compile option (additionalPropertiesInterfaceSuffix?) which, when not falsey, turnes on the export of interface {TypeName}{additionalPropertiesInterfaceSuffix}. I believe, however, that this should be a second PR, because this PR is "breaking" only in the most formal sense, since the currently generated interface does not compile anyways. (This PR still returns an interface, when no additionalProperties are present.)

G-Rath · 2022-07-31T20:42:46Z

in the example you gave only the finished type AdditionalProperties is exported ...
exporting the [...]KnownProperties-interface for each type with additionalProperties seems a tad unwieldy and pollutes – in my opinion – the namespace.

Sure in a perfect world it'd be great to less stuff while still being able to have the flexibility but this is what the language requires us to do in order for this change to not be breaking + retain that flexibility.

should not the aim of this package be the opposite
I can safely cast the object to (or assert it to be) the generated TypeScript-type!

While neither are terrible alternatives, they are not without costs: casting can introduce bugs if not understood and maintained properly because the whole point of them is to have TypeScript ignore type violations like missing properties and type mismatches, and assertions can have a significant runtime cost depending on the size of the objects (which people may shortcut with approximation assertions, which is fine but then you're in the first tradeoff again).

because this PR is "breaking" only in the most formal sense
This PR still returns an interface, when no additionalProperties are present.

Why is this being re-written as a type then?

I'm definitely happier if it doesn't change interfaces with no additionalProperties are present, but I still think because there's a solution that allows you to have your cake and eat it too is better than this current one - splitting that out into another config option IMO is bloating the library needlessly.

Please keep in mind that not everyone is validating their objects against the actual schemas at runtime - for example, over at https://github.com/octokit/webhooks we maintain schemas for all the webhook payloads which are used to generate the types that consumed by related packages; while the schemas are valid and can be used, they are not by the related octokit packages that are using the types. This is also the case for the rest of the GitHub API.

Declaration merging is very powerful here for the same reason its powerful over at DT: if we've made a mistake, or there's some new feature, and no one's gotten around to updating the schemas & types (we're pretty responsive but stuff happens), then people can easily amend the types using DM.

yourbuddyconner · 2022-12-12T22:58:05Z

Bumping this PR because it appears that TS declaration files are straight up broken (still). I applied this fix to my local node_modules, but like... come on.

How about less pedantic discussion about types and more fixy fixy?

EDIT: I now realize this isn't a serverless-namespaced package. I retract my snark, but getting this merged would still be helpful to me!

barbunzel · 2023-01-06T13:41:00Z

Would also love to have this merged!

bcherny

Sorry for the delay.

This should be implemented as a normalizer rule, and not as a special case in the parser. The rule should be: If a schema specifies non-boolean additionalProperties, then rewrite the schema as an allOf of the schema (without additionalProperties) and the additionalProperties. That would simplify the implementation, and address @G-Rath's point about backwards-compatibility for people taking advantage of declaration merging.

bcherny · 2023-05-04T09:55:12Z

test/__snapshots__/test/test.ts.md

+       * This interface was referenced by \`Callback\`'s JSON-Schema definition␊
+       * via the \`patternProperty\` "^x-".␊
+       */␊
+      [k: string]: unknown;␊


Can we improve this output?

roman-shandurenko · 2023-06-16T13:01:42Z

Can we get it merged, please? The issue has been open for 2 years and is still relevant and causes problems.

adambiggs · 2024-01-26T18:42:07Z

Pretty please?

fabrykowski added 2 commits April 21, 2021 13:15

Baseline

70c1c18

Export additionalProperties as intersection for complex types

38f4c5c

fabrykowski commented Apr 21, 2021

View reviewed changes

fredericbarthelet mentioned this pull request May 4, 2021

Typescript definition generation for resources.Resources has a compilation error due to Fn::Transform serverless/typescript#27

Open

Michael-Hering approved these changes Jun 22, 2021

View reviewed changes

Merge remote-tracking branch 'trunk/master'

c5f3124

# Conflicts: # test/__snapshots__/test/test.ts.md # test/__snapshots__/test/test.ts.snap

This was referenced Apr 6, 2022

additionalProperties can create invalid typings #299

Open

Broken generation with additionalProperties #356

Open

additionalProperties union clashes with defined properties #402

Open

Merge remote-tracking branch 'trunk/master'

c07b1e4

bcherny requested changes May 4, 2023

View reviewed changes

john30 mentioned this pull request May 15, 2023

Feature request: Configuration option to disable generation of [k: string]: unknown #335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export additionalProperties as intersection for complex types #383

Export additionalProperties as intersection for complex types #383

fabrykowski commented Apr 21, 2021 •

edited

fabrykowski Apr 21, 2021

bohjak commented May 6, 2021

fabrykowski commented May 6, 2021

fabrykowski commented May 15, 2021

fabrykowski commented Jul 1, 2021

Michael-Hering commented Jul 21, 2021 •

edited

maximseshuk commented Aug 11, 2021

luc-heyday commented Aug 12, 2021

electblake commented Aug 20, 2021

fabrykowski commented Oct 26, 2021 •

edited

macsj200 commented Nov 9, 2021

fabrykowski commented May 12, 2022

G-Rath commented May 12, 2022 •

edited

fabrykowski commented Jul 31, 2022

G-Rath commented Jul 31, 2022

yourbuddyconner commented Dec 12, 2022 •

edited

barbunzel commented Jan 6, 2023

bcherny left a comment •

edited

bcherny May 4, 2023

roman-shandurenko commented Jun 16, 2023

adambiggs commented Jan 26, 2024

Export additionalProperties as intersection for complex types #383

Are you sure you want to change the base?

Export additionalProperties as intersection for complex types #383

Conversation

fabrykowski commented Apr 21, 2021 • edited

fabrykowski Apr 21, 2021

Choose a reason for hiding this comment

bohjak commented May 6, 2021

fabrykowski commented May 6, 2021

fabrykowski commented May 15, 2021

fabrykowski commented Jul 1, 2021

Michael-Hering commented Jul 21, 2021 • edited

maximseshuk commented Aug 11, 2021

luc-heyday commented Aug 12, 2021

electblake commented Aug 20, 2021

fabrykowski commented Oct 26, 2021 • edited

macsj200 commented Nov 9, 2021

fabrykowski commented May 12, 2022

G-Rath commented May 12, 2022 • edited

fabrykowski commented Jul 31, 2022

G-Rath commented Jul 31, 2022

yourbuddyconner commented Dec 12, 2022 • edited

barbunzel commented Jan 6, 2023

bcherny left a comment • edited

Choose a reason for hiding this comment

bcherny May 4, 2023

Choose a reason for hiding this comment

roman-shandurenko commented Jun 16, 2023

adambiggs commented Jan 26, 2024

fabrykowski commented Apr 21, 2021 •

edited

Michael-Hering commented Jul 21, 2021 •

edited

fabrykowski commented Oct 26, 2021 •

edited

G-Rath commented May 12, 2022 •

edited

yourbuddyconner commented Dec 12, 2022 •

edited

bcherny left a comment •

edited