[TypeScript][Fetch] replace isomorphic fetch with portable fetch

[jeff-99]

Description of the PR

Fixes #6738

I replaced the isomorphic-fetch dependency with a well maintained fork (portable-fetch https://github.com/Knotis/portable-fetch) which supports React Native

[wing328]

@jeff-99 thanks for the PR but the CIs failed with the following:

> @swagger/typescript-fetch-petstore@1.0.0 postinstall /Users/travis/build/swagger-api/swagger-codegen/samples/client/petstore/typescript-fetch/builds/es6-target
> npm run build

> @swagger/typescript-fetch-petstore@1.0.0 build /Users/travis/build/swagger-api/swagger-codegen/samples/client/petstore/typescript-fetch/builds/es6-target
> tsc --outDir dist/

api.ts(16,32): error TS7016: Could not find a declaration file for module 'portable-fetch'. '/Users/travis/build/swagger-api/swagger-codegen/samples/client/petstore/typescript-fetch/builds/es6-target/node_modules/portable-fetch/fetch-npm-node.js' implicitly has an 'any' type.

  Try `npm install @types/portable-fetch` if it exists or add a new declaration (.d.ts) file containing `declare module 'portable-fetch';`

Please take a look when you've time.

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx

[jeff-99]

@wing328 , comitted the missing files. Build now seems to fail at the same step as the master branch.

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx

[TiFu]

Thanks for the PR.
The client security tests for typescript-fetch still contain isomorphic-fetch. Could you please regenerate the security-test? (see https://github.com/jeff-99/swagger-codegen/blob/feature/replace-isomorphic-fetch-with-portable-fetch/bin/security/typescript-fetch-petstore.sh)

[jeff-99]

@TiFu the security tests are regenerated

[wing328]

@jeff-99 I'll take another tomorrow and merge accordingly if no question from me. Thanks again for the PR.

@TiFu thanks for reviewing the change.

[wing328]

@jeff-99 thanks for the PR, which has been merged into master.

[masaeedu]

@wing328 @jeff-99 Is this change intentional? It reverts #6774.

[masaeedu]

What I don't understand is: if I run the tests today, without modifying anything, I find that this is changed back to Response. So something is broken in the way CI is being done for pull requests, because if the CI was working correctly it would reject this change.

I suspect the fact that there is a merge commit in here somehow confused things. In the future, it is probably best to require all pull-requests to be a single chain of commits that is rebased to the tip of master.

[masaeedu]

Aside from the comments above, it is probably not a good idea to constantly be modifying the fetch dependency in here. If you need a special fetch implementation to suit your needs, you can just inject it when creating your *API instance.

If it wasn't a breaking change I would suggest removing the default fetch from this generator entirely, and make fetch a required argument.

[kenisteward]

I believe the purpose of this change wasn't to just flip to a special implementation. I think there was a problem with isomorphic and they weren't resolving it so the pr changed to another that is a fork of isomorphic but fixes more issues . In general that seems fine but if there is a way to abstract the library from the client and let them know they need to choose a client that would be great. I still think having a default client is great though.

…

On Sat, Nov 18, 2017, 4:43 PM Asad Saeeduddin ***@***.***> wrote: Aside from the comments above, it is probably not a good idea to constantly be modifying the fetch dependency in here. If you need a special fetch implementation to suit your needs, you can just inject it when creating your *API instance. If it wasn't a breaking change I would suggest removing the default fetch from this generator entirely, and make fetch a required argument. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6739 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMPLteqBmpXqw-0qPRoqEpOrTHrh2KYkks5s3095gaJpZM4P-Q0e> .

[masaeedu]

@kenisteward The current implementation already abstracts the library from the fetch client. So when you instantiate your WhateverAPI, you can pass in an instance of fetch. For my use case I use node-fetch, which I import fetch from and inject as an argument for that parameter.

I think what would be better than having isomorphic-fetch or portable-fetch or whatever as a dependency of the generated library would be to simply use global fetch as a default value for the fetch API parameter. That way the user can just import whatever polyfill they prefer before they import the library and things will work fine.

There probably isn't a way to represent this in a good way in the types, but you could make it a runtime error if you instantiate *API and the fetch API argument ends up being undefined.

[TiFu]

Ideally we would hide all request code (e.g. fetch, and whatever other request methods are out there) behind one unified interface - making future integration with the other typescript code generators far easier and maybe provide one/some default implementations of that interface e.g. for node-fetch, isomorphic-fetch
I believe @kenisteward wanted to look into merging all ts generators (#5104).