Skip to content

Remove Unknown Type From Interfaces #294

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
grant opened this issue Jul 31, 2020 · 2 comments
Closed

Remove Unknown Type From Interfaces #294

grant opened this issue Jul 31, 2020 · 2 comments
Labels
module/lib Related to the main source code status/invalid This doesn't seem right status/wontfix This will not be worked on

Comments

@grant
Copy link
Member

grant commented Jul 31, 2020

Describe the Bug

The TS interfaces are pretty great.

I found a bug where the TS unknown type is used for some of the properties in src/event/interfaces.ts.

This is a problem because this TS type causes some issues:

Type 'unknown' is not assignable to type '{}'.ts(2322)


We should simply remove the unknown type as a possibility for these interfaces.


Test code:

const TEST_CLOUD_EVENT: CloudEventV1 = {
  specversion: '1.0',
  type: 'com.google.cloud.storage',
  source: 'https://github.com/GoogleCloudPlatform/functions-framework-nodejs',
  subject: 'test-subject',
  id: 'test-1234-1234',
  time: '2020-05-13T01:23:45Z',
  datacontenttype: 'application/json',
  data: {
    some: 'payload',
  },
};

describe('CloudEvents request to event function', () => {
  interface TestData {
    name: string;
    headers: { [key: string]: string | Date | undefined };
    body: {};
  }

  const testData: TestData[] = [
    {
      name: 'CloudEvents v1.0 structured content mode',
      headers: { 'Content-Type': 'application/cloudevents+json' },
      body: TEST_CLOUD_EVENT,
    },
    {
      name: 'CloudEvents v1.0 binary content mode',
      headers: {
        'Content-Type': 'application/cloudevents+json',
        'ce-specversion': TEST_CLOUD_EVENT.specversion,
        'ce-type': TEST_CLOUD_EVENT.type,
        'ce-source': TEST_CLOUD_EVENT.source,
        'ce-subject': TEST_CLOUD_EVENT.subject,
        'ce-id': TEST_CLOUD_EVENT.id,
        'ce-time': TEST_CLOUD_EVENT.time,
        'ce-datacontenttype': TEST_CLOUD_EVENT.datacontenttype,
      },
      body: TEST_CLOUD_EVENT.data,
    },
  ];
@lance
Copy link
Member

lance commented Aug 3, 2020

Hmm. I don't think it's that simple. As the specification states.

Description: The event payload. This specification does not place any restriction on the type of this information. It is encoded into a media format which is specified by the datacontenttype attribute (e.g. application/json), and adheres to the dataschema format when those respective attributes are present.

In your sample data, the TestData interface declares data to be {}. But there's no way to know for sure if that's actually the case. Even removing unknown, null and undefined from the CloudEvent#data type declaration, it still could be any of Record<string, unknown | string | number | boolean> | string | number | boolean; which would continue to be a problem for this example.

Type 'string | number | boolean | Record<string, string | number | boolean> | undefined' is not assignable to type '{}'.
  Type 'undefined' is not assignable to type '{}'.

313     body: TEST_CLOUD_EVENT.data,```

You can fix your example, by coercing the value

const testData: TestData[] = [
  {
    name: 'CloudEvents v1.0 structured content mode',
    headers: { 'Content-Type': 'application/cloudevents+json' },
    body: TEST_CLOUD_EVENT,
  },
  {
    name: 'CloudEvents v1.0 binary content mode',
    headers: {
      'Content-Type': 'application/cloudevents+json',
      'ce-specversion': TEST_CLOUD_EVENT.specversion,
      'ce-type': TEST_CLOUD_EVENT.type,
      'ce-source': TEST_CLOUD_EVENT.source,
      'ce-subject': TEST_CLOUD_EVENT.subject,
      'ce-id': TEST_CLOUD_EVENT.id,
      'ce-time': TEST_CLOUD_EVENT.time,
      'ce-datacontenttype': TEST_CLOUD_EVENT.datacontenttype,
    },
    body: TEST_CLOUD_EVENT.data as {},
  },
];

@lance lance added the module/lib Related to the main source code label Aug 3, 2020
@lance lance added status/invalid This doesn't seem right status/wontfix This will not be worked on labels Aug 11, 2020
@lance
Copy link
Member

lance commented Aug 17, 2020

Going ahead and closing this, as I don't believe it is an issue with the module

@lance lance closed this as completed Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/lib Related to the main source code status/invalid This doesn't seem right status/wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants