[Typescript-Fetch] Support additionalproperties, Enum, Auth and more. #6130

isman-usoh · 2017-07-21T03:28:36Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: master for non-breaking changes and 3.0.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Support additionalproperties and more.

Upgrade to TypeScript 2
Use type definition from npm instead of typings, typings is deprecation
Use Enum instead of String Literal Types
Use typescript es6 lib for target es5
Support additionalproperties
full support JSDoc
Add snapshot and npmRepository option
Update typescript-fetch run script for linux
Create typescript-fetch run script for windows

wing328 · 2017-07-21T14:15:00Z

@isman-usoh thanks for so many enhancements 👍

cc @Vrolijkx @taxpon @sebastianhaas @kenisteward @TiFu

Copying @mgechev as well since he's also working on some enhancements for TS Fetch generator.

- Upgrade to TypeScript 2 - Use type definition from npm instead of typings, typings is deprecation - Use Enum instead of String Literal Types - Use typescript es6 lib for target es5 - Support additionalproperties - Support JSDoc - Add snapshot and npmRepository option - Update typescript-fetch run script for linux - Create typescript-fetch run script for windows

- Fix circle run script - Fix duplicate query parameter

…ger-codegen into typescript-fetch

mgechev

Good job, that's a big improvement! I added a few comments, mostly style related, and a few with open questions.

mgechev · 2017-07-26T10:10:44Z

modules/swagger-codegen/src/main/resources/typescript-fetch/api.mustache

+// tslint:disable
+{{>licenseInfo}}
+
+import * as url                                 	    from "url";


More consistent indentation, currently it's mixture between tabs and spaces.

mgechev · 2017-07-26T10:10:53Z

modules/swagger-codegen/src/main/resources/typescript-fetch/api.mustache

+{{>licenseInfo}}
+
+import * as url                                 	    from "url";
+import * as isomorphicFetch                     	    from "isomorphic-fetch";


mgechev · 2017-07-26T10:11:46Z

modules/swagger-codegen/src/main/resources/typescript-fetch/api.mustache

+{{>licenseInfo}}
+
+import * as url                                 	    from "url";
+import * as isomorphicFetch                     	    from "isomorphic-fetch";


Do we need to introduce isomorphic-fetch as a dependency? Why not inject the fetch implementation from the outside instead?

isomorphic-fetch support run node.js and browser

If browser does not support Fetch API isomorphic-fetch use from fetch polyfill

Yes, sure, that's why it's called isomorphic. My point is, does it worth coupling to a specific dependency? Why not inject it from the outside, based on the declared interface?

mgechev · 2017-07-26T10:17:08Z

modules/swagger-codegen/src/main/resources/typescript-fetch/api.mustache

+ * @class BaseAPI
+ */
+export class BaseAPI {
+    public config: Configuration = new Configuration();


Maybe we can delay the instantiation of the Configuration object for after we've verified that the config parameter has not been passed.

mgechev · 2017-07-26T10:18:40Z

modules/swagger-codegen/src/main/resources/typescript-fetch/configuration.mustache

@@ -8,5 +11,6 @@ export class Configuration {
    };
    username: string;
    password: string;
-    accessToken: string;
+    accessToken: string | ((name: string) => string);
+    basePath: string;
 }


accessToken var >> return current token

accessToken function >> use for multiple Oauth, parameter auth name

basePath >> for override base path

Yes, this all looks cool. The point of my comment is that there's missing newline https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline.

mgechev · 2017-07-26T10:34:17Z

samples/client/petstore/typescript-fetch/builds/with-npm-version/README.md


-CAVEAT: Due to [privilege implications](https://docs.npmjs.com/misc/scripts#user), `npm` would skip all scripts if the user is `root`. You would need to manually run it with `npm run prepublish` or run `npm install --unsafe-perm`. 
+### publishing


Should start with uppercase being a title.

mgechev · 2017-07-26T10:34:21Z

samples/client/petstore/typescript-fetch/builds/with-npm-version/README.md


-You can also use `npm link` to link the module. However, this would not modify `package.json` of the installing project, as such you would need to relink every time you deploy that project. 
+### consuming


Should start with uppercase being a title.

mgechev · 2017-07-26T10:34:31Z

samples/client/petstore/typescript-fetch/builds/with-npm-version/configuration.ts

 export class Configuration {
    apiKey: {
        api_key: string;
    };
    username: string;
    password: string;
-    accessToken: string;
+    accessToken: string | ((name: string) => string);
+    basePath: string;
 }


mgechev · 2017-07-26T10:34:35Z

samples/client/petstore/typescript-fetch/builds/with-npm-version/index.ts

+ */
+
+export * from "./api";
+export * from "./configuration";


mgechev · 2017-07-26T10:35:05Z

samples/client/petstore/typescript-fetch/tests/default/tsconfig.json

-        "typings/browser",
-        "typings/main",
-        "typings/main.d.ts"
+        "node_modules"
    ]
 }


wing328 · 2017-08-02T08:51:57Z

If this looks good, I'll merge it tomorrow.

cc @Vrolijkx @taxpon @sebastianhaas @kenisteward @TiFu @mgechev

wing328 · 2017-08-03T09:52:05Z

@isman-usoh I got the following errors when trying to update the TS Fetch Petstore samples:

swagger-codegen|isman-usoh-typescript-fetch ⇒ ./bin/typescript-fetch-petstore-default.sh 
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=256M; support was removed in 8.0
[main] INFO io.swagger.parser.Swagger20Parser - reading from modules/swagger-codegen/src/test/resources/2_0/petstore.yaml
[main] ERROR io.swagger.codegen.AbstractGenerator - typescript-fetch/index.mustache (No such file or directory)
[main] ERROR io.swagger.codegen.AbstractGenerator - can't load template typescript-fetch/index.mustache
Exception in thread "main" java.lang.RuntimeException: Could not generate supporting file 'SupportingFile:
	templateFile: index.mustache
	folder: 
	destinationFilename: index.ts
'
	at io.swagger.codegen.DefaultGenerator.generateSupportingFiles(DefaultGenerator.java:617)
	at io.swagger.codegen.DefaultGenerator.generate(DefaultGenerator.java:734)
	at io.swagger.codegen.cmd.Generate.run(Generate.java:285)
	at io.swagger.codegen.SwaggerCodegen.main(SwaggerCodegen.java:35)
Caused by: java.lang.RuntimeException: can't load template typescript-fetch/index.mustache
	at io.swagger.codegen.AbstractGenerator.readTemplate(AbstractGenerator.java:51)
	at io.swagger.codegen.DefaultGenerator.generateSupportingFiles(DefaultGenerator.java:576)
	... 3 more

Should it be "TypeScript-Fetch/index.mustache" instead?

isman-usoh · 2017-08-04T11:34:35Z

@wing328 I rename template folder to lowercase, to be consistent other typescript project
modules/swagger-codegen/src/main/resources/TypeScript-Fetch > modules/swagger-codegen/src/main/resources/typescript-fetch

wing328 · 2017-08-04T11:46:27Z

@isman-usoh yes, I noticed that. Did you get any error when running the shell script or Windows batch file for TS Fetch Petstore clients?

isman-usoh · 2017-08-04T12:09:02Z

@wing328 No, I use windows os and run bin\windows\typescript-fetch-petstore.bat success, no error

bedag-moo · 2017-08-04T12:20:20Z

Why did you change from string literal types to enums? #6233 does the opposite (for the typescript-angular binding).

wing328 · 2017-08-04T17:13:20Z

Travis CI tests failed with errors. Here are some for reference:

> typescript-fetch-test@1.0.0 build /Users/travis/build/swagger-api/swagger-codegen/samples/client/petstore/typescript-fetch/tests/default

> tsc



test/PetApiFactory.ts(11,9): error TS2322: Type '{ api_key: string; }' is not assignable to type 'string | ((name: string) => string)'.

  Object literal may only specify known properties, and 'api_key' does not exist in type 'string | ((name: string) => string)'.

test/StoreApiFactory.ts(11,9): error TS2322: Type '{ api_key: string; }' is not assignable to type 'string | ((name: string) => string)'.

  Object literal may only specify known properties, and 'api_key' does not exist in type 'string | ((name: string) => string)'.

Ref: https://travis-ci.org/swagger-api/swagger-codegen/builds/261078041

Please take a look when you've time.

wing328 · 2017-08-04T17:14:24Z

bin/typescript-fetch-petstore-all.sh

@@ -1,5 +0,0 @@
-#!/bin/sh


I would prefer to keep the {lang}-petstore-all.sh convention so as to make it consistent with other languages.

https://stackoverflow.com/a/7517673/1077943

alexrashed · 2017-08-09T12:46:14Z

Are there any plans on supporting automatic redirects for OAuth2 with implicit flow using the mandatory authorizationUrl in the security definition of the swagger file?
If not, wouldn't it be better to make Configuration#accessToken and respectively ConfigurationParameters#accessToken async (return an Observable) to allow getting a new / refreshing an expired access token?

bedag-moo · 2017-08-09T13:08:24Z

Implicit flow involves navigating to the identity server, which destroys all javascript objects, so I don't see how that Observable could ever complete? Could you elaborate on how you think this could work?

kenisteward · 2017-08-09T13:23:18Z

In general things we implement here should be angular specific only. We can make it easily extensible but we shouldn't have dependencies on things like auth libraries however having easy config setup making it extensible to one should be fine

…

On Wed, Aug 9, 2017, 9:08 AM Adrian Moos ***@***.***> wrote: Implicit flow involves navigating to the identity server, which destroys all javascript objects, so I don't see how that Observable could ever complete? Could you elaborate on how you think this could work? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6130 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMPLtSNLlVWU3Y9eyptofq43vnGFYyHXks5sWa9UgaJpZM4Oe6q_> .

alexrashed · 2017-08-09T13:58:14Z

@bedag-moo I'm not an expert on the auth flow, but with activated CORS on the auth-server it should be possible to directly send the request to the (in the swagger.json) configured authorizationUrl f.e. to refresh the accessToken using the refreshToken. In order to implement such a lazy-refresh, the result of the accessToken function needs to be async.

bedag-moo · 2017-08-09T14:40:03Z

alexrashed: I think this is called the code flow (the implicit flow does not provide refresh tokens AFAIK). I agree this could be useful for code flow (or implicit flow in a nested iframe, come to think of it).

wing328 · 2017-08-12T14:42:55Z

Tests passed via https://travis-ci.org/swagger-api/swagger-codegen/builds/263778502

isman-usoh force-pushed the typescript-fetch branch from c0a4108 to b631a17 Compare July 21, 2017 05:33

wing328 mentioned this pull request Jul 21, 2017

[Typescript-Fetch] Support additionalproperties and more. #6093

Closed

3 tasks

wing328 added Client: TypeScript Enhancement: General labels Jul 21, 2017

wing328 added this to the v2.3.0 milestone Jul 21, 2017

isman-usoh force-pushed the typescript-fetch branch from 79c15b3 to 73b36e0 Compare July 23, 2017 16:39

isman-usoh and others added 5 commits July 23, 2017 23:38

Merge branch 'master' into typescript-fetch

9ed111a

Update and fix

f48b495

- Fix circle run script - Fix duplicate query parameter

Rename typescript-fetch folder to lowercase

e36b92c

Merge branch 'typescript-fetch' of https://github.com/isman-usoh/swag…

387d983

…ger-codegen into typescript-fetch

mgechev suggested changes Jul 26, 2017

View reviewed changes

isman-usoh force-pushed the typescript-fetch branch 2 times, most recently from 2fcf2b2 to 633e420 Compare July 27, 2017 04:17

wing328 mentioned this pull request Jul 27, 2017

Update TypeScript Fetch TypeScript version to 2.0.7 #4153

Closed

3 tasks

Fix for review and update new line end of file

633e420

isman-usoh changed the title ~~Support additionalproperties and more.~~ [Typescript-Fetch] Support additionalproperties, Enum, Auth and more. Jul 31, 2017

wing328 mentioned this pull request Aug 4, 2017

[TypeScript] Codegen class rename for Angular and AngularJS to reflect naming guidelines #6109

Merged

3 tasks

wing328 reviewed Aug 4, 2017

View reviewed changes

isman-usoh and others added 2 commits August 6, 2017 01:42

rename script to {lang}-petstore-all.sh and fix test

a70043f

Fix end of file

f763c1b

wing328 mentioned this pull request Aug 7, 2017

[Typscript][Fetch] add configuration to support authentication #4008

Merged

3 tasks

Fix override query string

cb0d5f7

https://stackoverflow.com/a/7517673/1077943

wing328 merged commit 3ecc374 into swagger-api:master Aug 25, 2017

janslow mentioned this pull request Feb 6, 2018

[typescript-fetch] 2.3.0 changes how parameters are passed to methods #7595

Open

jawa-the-hutt mentioned this pull request Mar 28, 2018

Typescript-Fetch add option to have parameters put in an Object #7930

Open

4 tasks

ivan-gomes mentioned this pull request Apr 26, 2018

[PYTHON] additionalProperties not supported #8075

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Typescript-Fetch] Support additionalproperties, Enum, Auth and more. #6130

[Typescript-Fetch] Support additionalproperties, Enum, Auth and more. #6130

isman-usoh commented Jul 21, 2017 •

edited

Loading

wing328 commented Jul 21, 2017

mgechev left a comment

mgechev Jul 26, 2017

mgechev Jul 26, 2017

mgechev Jul 26, 2017

isman-usoh Jul 26, 2017 •

edited

Loading

mgechev Jul 26, 2017

mgechev Jul 26, 2017

mgechev Jul 26, 2017

isman-usoh Jul 26, 2017

mgechev Jul 26, 2017

mgechev Jul 26, 2017

mgechev Jul 26, 2017

isman-usoh Jul 26, 2017

mgechev Jul 26, 2017

mgechev Jul 26, 2017

mgechev Jul 26, 2017

wing328 commented Aug 2, 2017

wing328 commented Aug 3, 2017

isman-usoh commented Aug 4, 2017

wing328 commented Aug 4, 2017

isman-usoh commented Aug 4, 2017 •

edited

Loading

bedag-moo commented Aug 4, 2017

wing328 commented Aug 4, 2017

wing328 Aug 4, 2017

isman-usoh Aug 5, 2017

alexrashed commented Aug 9, 2017

bedag-moo commented Aug 9, 2017

kenisteward commented Aug 9, 2017 via email

alexrashed commented Aug 9, 2017

bedag-moo commented Aug 9, 2017

wing328 commented Aug 12, 2017


		CAVEAT: Due to [privilege implications](https://docs.npmjs.com/misc/scripts#user), `npm` would skip all scripts if the user is `root`. You would need to manually run it with `npm run prepublish` or run `npm install --unsafe-perm`.
		### publishing


		You can also use `npm link` to link the module. However, this would not modify `package.json` of the installing project, as such you would need to relink every time you deploy that project.
		### consuming

[Typescript-Fetch] Support additionalproperties, Enum, Auth and more. #6130

[Typescript-Fetch] Support additionalproperties, Enum, Auth and more. #6130

Conversation

isman-usoh commented Jul 21, 2017 • edited Loading

PR checklist

Description of the PR

wing328 commented Jul 21, 2017

mgechev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isman-usoh Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wing328 commented Aug 2, 2017

wing328 commented Aug 3, 2017

isman-usoh commented Aug 4, 2017

wing328 commented Aug 4, 2017

isman-usoh commented Aug 4, 2017 • edited Loading

bedag-moo commented Aug 4, 2017

wing328 commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexrashed commented Aug 9, 2017

bedag-moo commented Aug 9, 2017

kenisteward commented Aug 9, 2017 via email

alexrashed commented Aug 9, 2017

bedag-moo commented Aug 9, 2017

wing328 commented Aug 12, 2017

isman-usoh commented Jul 21, 2017 •

edited

Loading

isman-usoh Jul 26, 2017 •

edited

Loading

isman-usoh commented Aug 4, 2017 •

edited

Loading