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

feat: Added types for typescript #116

Merged
merged 8 commits into from
Jul 28, 2020
Merged

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Jul 11, 2020

Description

  • Added types script to generate the types file
  • Changed the release github actions to re-generate the types on each release.
  • Changed the docs to accommodate the type generation

Related issue(s)
Fixes #93

@jonaslagoni jonaslagoni requested review from derberg and fmvilas July 11, 2020 22:24
@jonaslagoni jonaslagoni added the enhancement New feature or request label Jul 11, 2020
@jonaslagoni
Copy link
Member Author

I am unsure whether it is required to have a @types package or can just work as is 🤔

@jonaslagoni jonaslagoni changed the title Added types for the feat: Added types for the package Jul 11, 2020
@jonaslagoni jonaslagoni changed the title feat: Added types for the package feat: Added types for typescript Jul 11, 2020
@magicmatatjahu
Copy link
Member

@jonaslagoni Great job! @types package is usually for types, when authors of types are different than authors of package, or project is not maintance anymore. We should stay with types in this project, otherwise we would need to have the logic to push package under @types organization.

I have only one simple question. Why types are generated inside lib folder, not in the root?

@magicmatatjahu
Copy link
Member

@jonaslagoni Sorry 😅

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jul 12, 2020

@jonaslagoni Great job! @types package is usually for types, when authors of types are different than authors of package, or project is not maintance anymore. We should stay with types in this project, otherwise we would need to have the logic to push package under @types organization.

Ahh! Makes sense 😄

I have only one simple question. Why types are generated inside lib folder, not in the root?

Yea should probably be located in the root now that I think about it, since its generated and has no manual code added. I'll change it.

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jul 13, 2020

@jonaslagoni I fetched your PR, run npm run types and, packed parser to npm package, then I installed this package locally in another project and I cannot use any types from parser with error @asyncapi/parser/types.d.ts' is not a module, so I see two options:

  • leave the module but change it name to @asyncapi/parser - if module has name Parser or other than package name, then import doesn't work:

image

  • add export syntax (I don't know how) to those three methods in .d.ts` file.

Also I think that people want also types for Models, so this PR must include exported classes in module or with export syntax.

Those errors can be fixed by follow task :)

[TSD-JSDoc] utils.js:258:0 Failed to find parent of doclet 'utils.parseUrlVariables' using memberof 'utils', this is likely due to invalid JSDoc.
[TSD-JSDoc] utils.js:268:0 Failed to find parent of doclet 'utils.getMissingProps' using memberof 'utils', this is likely due to invalid JSDoc.
[TSD-JSDoc] utils.js:290:0 Failed to find parent of doclet 'utils.groupValidationErrors' using memberof 'utils', this is likely due to invalid JSDoc.

By the way, great job!

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jul 14, 2020

Thanks for the review 👍 I had skipped the step of actually compiling my test project as well which lead to some new problems as well as the one you noticed. I actually ignored it since I thought it was my compile options which did it 😅 Sorry about that.

  • leave the module but change it name to @asyncapi/parser - if module has name Parser or other than package name, then import doesn't work:

Ohh! Yea that worked perfectly 😄

So I managed to get everything generated under the module @asyncapi/parser using the @alias tag. If any new functions gets jsdoc and is only relevant to the parser itself it needs to be surrounded with @private otherwise it will be included in the types.d.ts file.

We might want to think about adding a test for the CI which creates/uses a typescript project and includes the parser and tries to compile it. Otherwise we end up with a faulty types.d.ts file without us knowing about it before it hits the developers. What you think about that? 🤔

Those errors can be fixed by follow task :)

The warnings has also been handled.

@magicmatatjahu everything checks out in my end now, can you review it once more? 😄

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jul 14, 2020

@jonaslagoni I tried again pack and load in another project, and everything works as aspected! I noticed yet, that project only exports from index:

  parse,
  parseFromUrl,
  registerSchemaParser,
  ParserError,
  AsyncAPIDocument,

so user have a types for models, but only can create AsyncAPIDocument model in project, Message, Schema etc is only reference for type, not constructor. At the moment this is not a problem for me, but should think about exporting other models to index. Maybe all of them? @derberg @fmvilas

Also, issue for update CONTRIBUTING.md about JSDoc will be needed because the parser has more and more things to do when writing code - JSDoc annotations for properly generated types.

About tests, good idea! But I don't know what it might look like. It should be little project, only with imports, so contributor will have to add a new type each time it appears.

Very good job! From my side it can be merged.

@jonaslagoni
Copy link
Member Author

so user have a types for models, but only can create AsyncAPIDocument model in project, Message, Schema etc is only reference for type, not constructor. At the moment this is not a problem for me, but should think about exporting other models to index. Maybe all of them? @derberg @fmvilas

In which context or use-case would they want to create their own instance of AsyncAPIDocument or any of the other classes? 🤔

About tests, good idea! But I don't know what it might look like. It should be little project, only with imports, so contributor will have to add a new type each time it appears.

This could be as simple as just a compile test, if the parser can be imported and the project can be compiled all good. Since most common error we can make is a faulty jsdoc which renders the project uncompilable in typescript.

@magicmatatjahu
Copy link
Member

I just think so. As a developer, I don't like having a closed door if I need custom logic 😄

magicmatatjahu
magicmatatjahu previously approved these changes Jul 14, 2020
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it in small ts-node app and it works! Great job.

@derberg
Copy link
Member

derberg commented Jul 27, 2020

@jonaslagoni yo man, you have come conflicts there 😉

fmvilas
fmvilas previously approved these changes Jul 27, 2020
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff 👏

@jonaslagoni jonaslagoni dismissed stale reviews from fmvilas and magicmatatjahu via 4aaa5aa July 27, 2020 16:36
@jonaslagoni
Copy link
Member Author

@jonaslagoni yo man, you have come conflicts there 😉

Solved :)

@jonaslagoni jonaslagoni requested a review from fmvilas July 27, 2020 16:37
@derberg
Copy link
Member

derberg commented Jul 27, 2020

@jonaslagoni linter fails with some error in asyncapi.js

@jonaslagoni
Copy link
Member Author

@jonaslagoni linter fails with some error in asyncapi.js

Fixed right before you saw it too 😄

@derberg
Copy link
Member

derberg commented Jul 27, 2020

@jonaslagoni did you think maybe how we can make sure it works overtime? how to test it? I'm thinking about it as this is something we need to solve for the browser bundle generation and maybe this could be somehow combined, at least the mechanism behind it

@jonaslagoni
Copy link
Member Author

We might want to think about adding a test for the CI which creates/uses a typescript project and includes the parser and tries to compile it. Otherwise we end up with a faulty types.d.ts file without us knowing about it before it hits the developers. What you think about that? 🤔

@derberg wrote something about it earlier. We could catch many of the errors by just using the types.d.ts file in Typescript project. Other then that not sure.

@derberg
Copy link
Member

derberg commented Jul 27, 2020

@jonaslagoni so it could be a react project in typescript that parses dummy spec file. We would just add also a simple script with https://github.com/puppeteer/puppeteer that would make sure there are no console errors and some basic elements are visible. So one project to fix both test challenges?

@derberg
Copy link
Member

derberg commented Jul 28, 2020

@jonaslagoni I created an issue for this #130

derberg
derberg previously approved these changes Jul 28, 2020
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve, and looking that @magicmatatjahu and @fmvilas approved before merge conflicts, we can merge now

@derberg
Copy link
Member

derberg commented Jul 28, 2020

@jonaslagoni before I merge, I noticed you're adding now @private to generate typescript types properly I guess, right? then please add it to also to the functions that you merged into your PR after solving conflicts

@jonaslagoni
Copy link
Member Author

@derberg done 👍

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 1 Security Hotspot to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done mate!

@derberg derberg merged commit 7102185 into asyncapi:master Jul 28, 2020
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.28.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Types for typescript?
5 participants