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

(Support?) Type errors for dev environment & future plans #85

Open
max-scopp opened this issue Jun 22, 2019 · 4 comments
Open

(Support?) Type errors for dev environment & future plans #85

max-scopp opened this issue Jun 22, 2019 · 4 comments

Comments

@max-scopp
Copy link

I'm looking for a good fork of wcandillon's work. This seems a good place to start off - especially since it's hosted by an Microsoft member.

Sadly, I'm required to have an URL-to-method lookup which may (probably) requires a small rewrite of this module. I'm also required to have properly used return types (partly true of #77).

Per readme, I've been trying to fire up the dev environment using npm run build:watch, which works, but I'm getting a ton of type errors.

At this point in time, I do not know if this is on a todo - wasn't able to find any hints on this.
I will be able to fix this, but I'd like to have this merged into here, rather than creating yet another fork that get's on npm.

In any case, I have to fix this nevertheless before implementing my wishes.
In case my feature requirements do not meet your opinion, I'd like to keep the core for the both of us the same (and putting my feature on-top).

Looking forward for clarification. Would love to contribute.

@mtennoe
Copy link
Owner

mtennoe commented Jun 23, 2019

Hi @max-scopp!

Seems like you closed this issue. I guess it is still relevant? Were you able to get the environment running? If not, what kind of type errors are you receiving?

@max-scopp
Copy link
Author

Thanks for your Response, I've gotten myself a drink yesterday and reworked the whole template.

I was able to get the environment running and no longer have any errors, if I remember right, they where just jest-specific type problems anyways. Can't reproduce anymore. After checking out the master I now only get Types of property 'additionalProperties' are incompatible. Type 'boolean' is not assignable to type 'SwaggerType | undefined'..

Anyways, my Version now includes the following features:

  • Removed superagent, replaced with native fetch (node-fetch as server variant)
  • Index-based endpoint mapping
  • changed URL builder(s) {{&methodName}}URL to native URL class
    • also added missing JSDoc
  • Endpoint methods are using the {{&methodName}}URL URL builder (less code duplication)
  • Efficient reverse URL lookup (getMethodForUrl(url: string, method?: string))
  • Request responses now align with the native Response, except body is kept type-safe
    • The Response is also a real class SwaggerResponse, rather than a simple object
  • Generally the clients feel lighter
    • not validated, just less repeating code as everything is defined in one huge Map and everything else reference to it

I was wondering why specifically the decision was to use superagent. It might be small, but it still is a dependency and a non-standard brewed api. At least with node-fetch it's practically just a polyfill for node.

Today I'm continuing with cleanup, if you're interested in my version, just hit me up. Sadly I would then need an e-mail to invite you on our internal gitlab.

I bet I've gotten something wrong with the parameters and parameters.$queryParameters.
I'm still relatively new to swagger, I tried to stay close to the original as possible, but the template contained a few strange lines.

But it works smooth so far. Would love if you take a look at it!

@mtennoe
Copy link
Owner

mtennoe commented Jun 24, 2019

Cool!

Re. superagent - This is from the original fork, and we havent removed it. In a separate project I am working on using this project we use custom templates where we have removed the superagent usage and rather use XMLHttpRequests. The use of fetch also makes sense, but comes at the cost of needing to polyfill for IE. I don't feel strongly for keeping superagent (except backwards compatibility). @scottc, @Markionium, @Kosta-Github - Any opinions here?

Re. Making SwaggerResponse a class - Does it have to be a class? Making it a class has the downside of resulting in a significantly higher bundle size than just being a simple type

Would be cool to try to incorporate your improvements into the project (at least most of them)!

@mtennoe mtennoe reopened this Jun 24, 2019
@scottc
Copy link

scottc commented Jul 31, 2019

RE: XMLHttpRequests

I'm using axios in my custom template.
I would also want to have node support, for SSR.

There are some other similar lightweight isomorphic-fetch polyfill libraries, I'm not particularly attached to any specific implementation lib either. As long as it works in all commonly used browsers, and other such run-time environments.

RE: code generation.

My preference would be using static types, and producing as little javascipt code as possible, to reduce bundle sizes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants