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

fix: add two type definitions to SCM and fix their parameters #189

Closed
wants to merge 1 commit into from

Conversation

lance
Copy link
Member

@lance lance commented May 27, 2020

It seems that tsc doesn't recognize when a class or function has a single
options parameter that is required but properties within it are not.
This corrects the output from tsc type definition generation and adds
the two files affected by this to repo.

This might not be the best way to go, but it is a quick fix that we should at least discuss.

Fixes: #188

@lance lance added the type/bug Something isn't working label May 27, 2020
@lance lance requested a review from a team May 27, 2020 22:21
@lance lance self-assigned this May 27, 2020
@lance lance requested review from danbev, grant and fabiojose and removed request for a team May 27, 2020 22:21
It seems that `tsc` doesn't recognize when a class or function has a single
`options` parameter that is required but properties within it are not.
This corrects the output from `tsc` type definition generation and adds
the two files affected by this to repo.

Fixes: cloudevents#188
Signed-off-by: Lance Ball <lball@redhat.com>
@grant
Copy link
Member

grant commented May 27, 2020

This is certainly one solution. Thanks for trying to get the types in here!

However I think a more elegant solution would just be to not create new files (.d.ts) that are hand-modified, and to just change the files to normal files (.ts without the (.d). By excluding certain .js files, it looks like we are manually creating these type definitions, which is not sustainable or conventional.

I think we can all agree that we shouldn't include generated files in source control.
Happy to hear thoughts around this.

@lance
Copy link
Member Author

lance commented May 27, 2020

Thanks for the feedback. Can you put together a PR that illustrates what you are talking about and perhaps uses an interface to address it?

@grant
Copy link
Member

grant commented May 27, 2020

I'd imagine something like this:

constructor(cloudEvent: CloudEventV1 = SPEC_V1) {
 ...
}

In a cloudevent.ts file:
https://github.com/cloudevents/sdk-javascript/blob/master/lib/cloudevent.js#L29


The source for CloudEventV1 would be something like this:
#184 (comment)

Unfortunately I don't think we can add optional types without changing the extension or manually creating types as suggested in this PR.

I'd rather rename the file extension as generated-then-hand-modified files are hard to keep up-to-date.

@lance
Copy link
Member Author

lance commented May 27, 2020

@grant if you can put together a PR for that I would appreciate it. I think that will be easiest for me to understand how it all fits together, and to experiment with it from both a pure JS project and a pure TS project.

@grant
Copy link
Member

grant commented May 27, 2020

@grant if you can put together a PR for that I would appreciate it. I think that will be easiest for me to understand how it all fits together, and to experiment with it from both a pure JS project and a pure TS project.

I'm happy to put a PR together if it will be accepted. However, I don't know if you understand what I'm suggesting. This would be my 3rd attempt at introducing .ts to CloudEvents, and I have moved on to other languages until we can solve this.

From a technical standpoint, I would like to ensure we understand:

  • We're OK with these filename change:
    • lib/cloudevent.js -> lib/cloudevent.ts
    • lib/bindings/http/http_emitter.js -> lib/bindings/http/http_emitter.ts
  • We're OK with using a uniquely TypeScript feature, interface to support optional types in objects. Example: CloudEvent JSON Autocompletion without Class Wrapper #184 (comment)
  • We're willing to use continually use tsc --watch to continuously build .ts files into a .js files.

If we are not happy with those understandings, I can't see a technical way to solve this problem and can't contribute. if we're OK with that, I am free Friday to collaborate and put a PR together.

@lance
Copy link
Member Author

lance commented May 27, 2020

Yes I think this could be ok. I am not too concerned about the filename changes. I don’t love the continuous tsc but I can live with it. What I want to better understand is how an interface fits in. What is the filename? How does it affect (if at all) the way a pure JS developer interacts with the SDK? Do any of the other type definitions need to know about it? Etc.

I meant what I said when I said #155 could be the start of a gradual movement towards TS. And I admit that this comes from ignorance on my part. But I don’t think that I would have been able to get a 2.0 release out as quickly as I did if I had to also be learning the machinations of typescript in the meantime.

I’m happy to start making that journey right now.

@grant
Copy link
Member

grant commented May 28, 2020

Hey Lance, I'll try to answer some of the questions.
TL;DR; Javascript users will get the same experience. TS developers and SDK developers will get better autocompletion and less code duplication.

Yes I think this could be ok. I am not too concerned about the filename changes. I don’t love the continuous tsc but I can live with it.

OK. Yeah it's sorta bothersome in the beginning, but it's really lightweight and provides a pretty good devX imo.

What I want to better understand is how an interface fits in. What is the filename?

So, right now we are generating .ts files with tsc on js files. The generated files are named *.d.ts, still .ts files. Tsc infers the types and generates declarations. Ctrl+click to see the definition of the types. OK autocompletion but not great. I know it's painful the first time you look at a .ts file with interfaces, but it's worth understanding interfaces.

A TypeScript interface provides structure to a plain old JavaScript object. It's used by editors like VS Code and others, even if you're using plain JavaScript. Some npm modules.

It affects the SDK code writing { id, source, type, dataContentType, time, subject, dataSchema, schemaURL, dataContentEncoding, data, specversion } multiple times (no wrapper class).

When we publish the npm module, we publish our code which has a package.json file. This file can have a types property. This types property is used by editors to view the types.

How does it affect (if at all) the way a pure JS developer interacts with the SDK? Do any of the other type definitions need to know about it? Etc.

It doesn't affect the pure JS end developer. Pure JS developers do however often use editors like VS Code that provides inference via TS. TypeScript does not attempt to force the JS world to type TypeScript, it's strictly optional because it is a strict language superset.

I meant what I said when I said #155 could be the start of a gradual movement towards TS. And I admit that this comes from ignorance on my part. But I don’t think that I would have been able to get a 2.0 release out as quickly as I did if I had to also be learning the machinations of typescript in the meantime.

Great. It's fine to have everyone learn and contribute to the project with their own perspectives and requirements.

I’m happy to start making that journey right now.

Great to hear.

@lance
Copy link
Member Author

lance commented May 28, 2020

Closing in favor of #194

@lance lance closed this May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated type definitions don't declare optional parameters
2 participants