-
Notifications
You must be signed in to change notification settings - Fork 69
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
TypeScript types #9
Comments
@micheleangioni Could you make a PR with this? If you can't, please write an example using some piece of code from sdk. |
I do not have the complete library definitions. I have the definitions of the These are the definitions I wrote:
|
@micheleangioni Now I understood. Do you want to join us and work in this issue? |
Yes, sure. I hope to find some time to write the declarations also of the remaining classes, hopefully in a couple of weeks :) |
@micheleangioni, would be awesome! Novice question, how to use the type definition above w/ Thanks. |
@micheleangioni, I had to change the typing to the following to match https://gist.github.com/cwallsfdc/183c2df8dad88964efa957217b12c496 |
@cwallsfdc Can you help us in how to publish this typing in NPM? |
Honestly, I think a better approach is to convert to Typescript and include the typings in the If in agreement, I can post a PR w/ Typescript changes. We'd then publish a new Thoughts? |
LGTM! @cwallsfdc, go ahead! Perform your PR! |
Google Cloud Functions uses it's own TypeScript types because this SDK does not have them. Google Cloud Functions Framework: |
Answering questions from email:
Yes. When we publish to
We can publish this npm module instead of using DefinitelyTyped. See https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html Just let me know when |
@grant Thanks for the answers. Because of the changes that are coming from the spec, I think that is more prudence to publish the types using Definitely Typed. This will help the projects written in TypeScript and we save time to model the stuff for the future of the sdk (writing in ts) and to work in the incoming spec changes. It's fair if we make voting:
What do you thing about that @grant ? rigth now, |
|
Thanks for leading this charge @fabiojose. I agree with @grant. |
Vote b if you are in favor of to migrate the entire codebase from JavaScript to TypeScript @grant Thanks for your contribution! Let's code? |
Will you code for this issue? Just to know. |
I can't continue since this repo doesn't seem to follow the CNCF standards of having reviews. Need to check on Slack and with others first. |
Nice! This is a great opportunity. You can become a new commiter and helps us to review. At this moment I am the only one: maintainer and commiter. Because of that, I guess, you got that impression of loose standards. Come, join us! Make this better! |
@grant can you elaborate? As @fabiojose mentioned, we do try to follow "normal" review processes (see the golang repo, it has more people involved) but if there's only one person active in the repo then it's hard to require multiple people for reviews :-) |
In order for me to contribute, I need master to not change as fast as it is. The commits made to master are not PRs, they are direct to master. I would like to convert this repo to typescript, so that I can use the SDK with Cloud Functions, but last time I tried, there were commits to master that conflicted with my conversion. All commits in this repo should be tied to a PR and all PRs are squashed so we can see the history change. Aside, I don't really understand why we should have exceptions for reviews with certain import repos. |
I would like to join migrating cloudevents to typescript! |
@grant, in the early days I used gitflow to merge to master, without PRs. Never commit directly. Now we are using PRs, but I must approve by myself. You can help us in the review of that PRs. Is that makes senses to you? |
Makes sense. I can look into converting this package again. It needs a lot of major changes though. |
Folks, it's not the best, but now we have types for version 1.0.0 |
A remark from a (rather inexperienced) typescript user: the provided types are not really flexible as they are more of an interface than a type definition. I think the whole structure would be much more versatile (i.e. usable in / connectable to other libraries) if you split the event builder pattern from the bare data model and use basic object attributes instead of getters and setters in the type definition. In my case, I wanted to receive the events from rabbitmq and store them in mongodb but both libraries expect type definitions with bare objects (and for instance iterate over their keys). |
Hi @fabiojose, At Google, we don't use this CloudEvent SDK (even though we use CloudEvents with Node) because of some of the issues described above. I would like to improve this situation but I need to be able to contribute to this repo and use best practices. To restart trying with this repo, I've created a simple PR: After that, I would love to help with doing things like simplifying the multiple versions we have in this repo and conversion to modern TypeScript. |
Yeah, I guess it just depends on preference. Personally, I prefer just javascript because there is a wider audience of developers who author code in JS. So in theory, the larger opportunity for OS contributions lies in pure JS. Additionally, I think Typescript has a larger learning curve and can be trickier for folks to pick up. Both work and I have no dog in this fight 😃. Pros and cons both ways |
I finally found time to read this whole thread! oy vey! :-) To make sure I understand things since (as I've said) I don't know TS at all and don't do anything with JS outside of a browser, early on in the thread it seemed like there were two concerns from @lance w.r.t. the proposed direction: And, as I understand it, TS is a compiled language in the sense that it needs to be converted from TS to JS before it is executed (interpreted) at runtime. Sound ok so far? If so, I have just a couple of questions:
I'm guessing the answer to all 3 bullets is basically "no" which is why we landed on the "new SDK" decision. But I wanted to make sure I'm up to speed. How far off am I? :-) |
Thanks for taking the time - and on a Friday night, even. 😄
Not really - those were concerns that were attributed to me by others. Really what I care about is maintainer productivity and the larger pool of potential contributors (and alienating existing contributors who have only recently started to submit code (including myself)).
Correct.
Yes - see David's comment above. Personally, I think this is the best path forward for a possible migration to TypeScript in the future. This is the compromise I have recently suggested. I honestly do not want this community fractured and I'm trying to be open about this. My preference is pure JS. Clearly, that feeling is not universal. ☯️. I personally would be most comfortable with a graduated approach using the methods suggested by David. Concerns about doc comments drifting from implementation are valid, but there are ways to deal with this using tools and processes. |
I have limited TS knowledge and I have no room for TS learning in my free time to be able to be comfortable in contributing. |
Yes. Those are the two main points, TypeScript is required for many projects including those at Google and Microsoft. The best path forward is a separate implementation with TypeScript. If there is value in merging projects at a later point, that sounds fine. |
@grant Can you explain the specifics of the Typescript requirement? Are guaranteed to be correct typescript types are not enough? The js=>ts renaming doesn't seem to be necessary with the proper tsconfig and the addition of watch/build using tsc and enforced by CI has largely been ceded. Do I appear wrong on these points? Insisting on these two points despite direct statement that they are not the concern is a curious behavior. You seem to be implying that only JavaScript features would be used. I'm curious how you expect that could be affected. Do you expect contributors from across the internet to forgo using TypeScript features despite project conversion and labeling as typescript? Is there a mechanism you have in mind for enforcing that? Are you willing to agree to it's use? |
Sure. As a developer:
There are a few projects that could use an SDK for CloudEvents in Node. Here is an example project that could use the SDK with TypeScript: Example at Googlehttps://www.npmjs.com/package/@google-cloud/functions-framework It powers serverless functions for Google Cloud. We don't use this module to accept/parse HTTP requests with CloudEvents due to a few issues, one being lack of TS support. We require autocompletion and static checks to ensure that the module is running correctly. It's vital for ensuring we don't crash our product for thousands of customers on a bad deploy. Many other companies use this superset language. It's Literally NodeIt's literally no different, please browse the code of the TypeScript version: It's IncrementalYou don't need to learn a new language. We can enforce not using non-JS features. It's an industry-wide standard that you're probably using right nowIf someone was to magically enforce types and IDE integration with types that are not from TypeScript, that's fine. That's really the point of TypeScript though. There is a whole ecosystem and company support around TypeScript. Example players supporting TS: https://tsconf.io/ I can't find any project that does anything similar. I assume many people use VS Code for code editing right? That heavily uses TypeScript. Other Questions
Editors like VS Code will do it's best with just
I am saying that the main issue we're facing is the fear of change. We don't have to change any of the JavaScript if we don't want to to use TypeScript. There aren't that many contributors to this project for us to know to not accept PRs with extra TS features. We don't even consistently use ES6. I'm willing to agree on not using TypeScript features and we can add a simple lint file to enforce this via |
Finally, caught up with all of this thread and the slack discussion. :-) As I brought up the concern in last week's call and now will say more strongly... I'm against having two separate, "official" "Javascript" sdks. That's just plain bad optics and makes for confusion, bugs, interop, support, etc. issues for users/consumers/providers of CE. |
If I understand the camps' positions, @loopingz's code translation and some simple policies such as the tslint enforcement seem like a fair place to start so that we can remain as single JS SDK for CE. As someone who has no personal stake in the pure-JS vs. TS approach, I personally hope that people in the various camps are willing to at least try a joint approach openly before splitting into two separate, competing efforts. |
Hi @johnm, thanks for your opinion, but there are a couple issues. I literally cannot use this project unless it fully supports TypeScript. I oppose two separate JS-like repos too on the surface. I'm happy to merge separate repos if possible. What do you expect me to do? This is a deal-blocker. We currently have separate efforts, literal code duplication already, since there is no TypeScript support. I'm obviously not as happy to continue a separate effort outside of a this repo or outside of this GitHub org. |
Two concerns I have heard are exclusion (or even just reduced welcome) and a potentially reduced contributor pool and thereby speed of improvement and community inclusion. I understand that typescript is a superset and have indicated personal comfort with it (by admitting use). To be transparent I would welcome a full transition an use of typescript constructs personally but my personal mores are unimportant in this case. Depending on how we introduce typescript (if we do at all) we could create externalities. This includes factors such as a subset of the population seeing unexpected constructs and be put off regardless of what size the learning curve might be. Thank you for sharing the ability of tslint to enforce sticking to core JavaScript. I'm curious if using that sufficient compromise for those who would like to stick with vanilla JavaScript (@lance or anyone else?). I'm also a bit mystified by the assertion that the technical/concrete requirements would care at all about a file name. If we look at
It turns out that (as a stand in for an important subset of users) your needs matter too. What we are talking through are the deal-making constraints and options. I apologize that my ignorance is forcing me to ask you to explain options and a bit of how our potential choices interact and what the choices available to us are. I can imagine it is a little frustrating but I can only be where I am, please apply patience as you can. |
Hm... Did you respond before reading my second comment? I hope that made clear, as I also commented in the call, was that it seems that there's a combined approach place to come together other. Based on that, are there still deal breakers? |
Well, thanks for all the replies and trying to figure this out @erikerikson. Sorry @johnm, I was reacting a bit. This issue has been open for over a year with hundreds of commits to this project directly to |
No worries, I understand the frustration as I've been in similar situations previously. FWIW, I separated the two different aspects on purpose. There are the problems at technology level but also at the project level (of the impact on the users/etc). I think the context of the project should be keep in mind when dealing with the tradeoffs in making the technology decisions. |
May I come in and talk with you about our strongly typed savior, Rust & Wasm? ;-) :-) |
I was under the belief that using
If type checking is enabled with
I might say reluctance to change. There are existing contributors (including myself) who are reluctant to switch fully to TS for a number of reasons. But I don't know that you are representing them in good faith. A reduced contributor pool and alienation of existing contributors are what I care most about. I really don't like the required transpilation stage in order to actually have a JavaScript engine be capable of reading my code, but I can live with that, I suppose. I do like the fact that tslint can enforce pure JS. But I wonder, what's the point of actually converting to TS in that case if we can achieve the user-facing goals (knowing the properties of a CE, autocompletion, linting, etc) with As @johnm has suggested, I would like to consider a path forward that starts small, using tsc to check the code, jsdoc to enable both IDE autocomplete and help tsc, and type declaration generation. Do this with an eye towards eventually moving to TS when this whole thing has simmered down a bit. I understand that there are potential benefits to TS. But there are reasons to take it slowly. I would like to think that this is a fair compromise. To be quite honest, I would be surprised if Google immediately started using this project if we did nothing but switch to TypeScript by changing file names and adding a watch command. Those first compromising steps seem necessary in any case. Rather than a switch to TypeScript, my most pressing concerns are:
Honestly, I think these add much more value than renaming a bunch of files. I'd like to focus on these before making any major repository-wide changes if possible. |
One of the big point of TypeScript is to not have to duplicate your code inside of docs. Right now we don't have many comments in the code. The suggestion here is to add JSDoc and annotate types everywhere. That's going to create a lot of boilerplate. There are 2 options:
The above comment chooses option 1. I'm suggesting 2. Again, switching to TypeScript is not a big change that will prevent you from having to write all that JSDoc.
I literally work for Google, for Events, and have said multiple times I'd use this module if it was useful and supported TypeScript. See #9 (comment) |
This commit introduces TypeScript checks and generates type declarations for the existing JavaScript codebase using `tsc` prior to running the linter task. Ref: cloudevents#9
This commit introduces TypeScript checks and generates type declarations for the existing JavaScript codebase using `tsc` prior to running the linter task. Ref: cloudevents#9 Signed-off-by: Lance Ball <lball@redhat.com>
This commit introduces TypeScript checks and generates type declarations for the existing JavaScript codebase using `tsc` prior to running the linter task. Ref: cloudevents#9 Signed-off-by: Lance Ball <lball@redhat.com>
This commit introduces TypeScript checks and generates type declarations for the existing JavaScript codebase using `tsc` prior to running the linter task. Ref: cloudevents#9 Signed-off-by: Lance Ball <lball@redhat.com>
This commit introduces TypeScript checks and generates type declarations for the existing JavaScript codebase using `tsc` prior to running the linter task. Ref: cloudevents#9 Signed-off-by: Lance Ball <lball@redhat.com>
This commit introduces TypeScript checks and generates type declarations for the existing JavaScript codebase using `tsc` prior to running the linter task. Ref: cloudevents#9 Signed-off-by: Lance Ball <lball@redhat.com>
This commit introduces TypeScript checks and generates type declarations for the existing JavaScript codebase using `tsc` prior to running the linter task. Ref: cloudevents#9 Signed-off-by: Lance Ball <lball@redhat.com>
Fixed in: #155 |
The TypeScript declaration package is missing in https://www.npmjs.com/~types .
More information on this: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html
This is very important to ensure a proper diffusion of the cloudevents standards.
The text was updated successfully, but these errors were encountered: