-
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
build: add tsc type checks and emit declarations in the ci/test pipeline #155
Conversation
9466f73
to
704278b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have limited knowledge in TS.
But since I was requested to review the PR I'm approving based on:
but keeps the code base as pure JS mainly by just keeping the entry point index.js
/cc @johnm @cwallsfdc @erikerikson @loopingz @DavidWells - feedback from you all is welcome. |
If we're going the "never change
I'm surprised that we're ok with generated |
@grant again I think you are not representing my position in good faith. I have never said that we are going the "never change I humbly asked what was the right thing to do with those |
@lance Oh, I'm not trying to be snarky. I'm trying to get TypeScript support here. It's common to not include generated code in git repositories, as it can simply be generated from the source code again. In this case, we are generating files with the extension Emitting declarations is fine for local development but shouldn't be seen on GitHub or npm. Again, this is a weird way to beat around the bush and use legacy TypeScript features for unideal projects. Ideally, like most modules, we don't need any See the handbook for more details: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we don't need .d.ts
files.
But if we have them, they shouldn't be checked into source control.
OK - I will update it.
The handbook you link to is all about publishing your |
Yes. The link has really useful guides in using TypeScript, including best practices. The issue about files on npm is just one of the comments on the added It will clearly recommend using TypeScript |
As I wrote in the description 🤷♂️
|
We can't just ignore people's pull requests and create/merge your own without discussion! Why can't we just pool together experience and collaborate! |
@grant I'm not ignoring your PR - I'm proposing an alternative because we have been doing nothing but discussing this for a while now. There has been a lot of discussion. I wish you could acknowledge that I'm at least trying to find some common ground here. |
Thanks Lance for listening through these discussions. Can you review the PR and not create new PRs that conflict with old ones? |
Thank you @lance for providing this as an alternative and clearly marking it as such. @grant it's not clear where Lance has merged his own PRs (although pretty obvious he created this one). As he's indicated the point of this is to give substance to the discussion that has been ongoing and he does seem to be acting in good faith. |
@lance, I appreciate that you are compromising. But, if you are willing to run Can we move to |
@cwallsfdc can you help me understand what doing that actually achieves if the stated intent is to stick with pure JS syntax? Why go through the transpilation step if we're going from pure JS in a I realize that you and others may find it hard to understand why I don't want to immediately move to Isn't the goal to have a better experience for users of this SDK? If so, how does my alternative approach not achieve that in an equivalent way while also allowing room to move forward with something further along the continuum later? |
It seems that this is a fundamental misunderstanding on my part. It's become quite clear that this is not the actual intent. |
Including the JsDocs for immediately answering semantic questions non-experts and those trying out the SDK may have. Regardless of the final approach to tooling, I expect that enriched JsDocs will be a positive addition to support adoption and ongoing usage. |
It is the intent. What do you mean it became quite clear that intention changed? You are insinuating malintent. I can imagine we could add a TS interface for a CloudEvent object in the future if agreed upon. How else will we see object autocompletion without getters and setters? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good. Thanks for the PR @lance!
Let's go with this.
7cc77aa
to
83bc786
Compare
@grant I honestly do appreciate you making this compromise, helping us both find a path forward towards making this project what we both want it to be -- a high quality, stable and and user-friendly module. Though our conversation has been heated at times, I look forward to putting all of that behind us and working together with you and the rest of the community to make that happen. |
b242d28
to
87265f8
Compare
87265f8
to
4604fa1
Compare
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>
Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
4604fa1
to
e6cc099
Compare
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. This is similar to @grant's pull request, but keeps the code base as pure JS mainly by just keeping the entry pointindex.js
instead of.ts
.I am proposing this as a very slight alternative. Since I'm not fully familiar with the TypeScript tooling, I wasn't sure if the type declaration files should live side-by-side with the JS or not, but I assume they should be kept in SCM. Feedback welcome.
Ref: #9