Typescript-fetch: file upload and Typescript dependency updates #4852

russtacular · 2017-02-24T20:58:09Z

PR checklist

Read the contribution guildelines.
Ran the shell/batch 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)
Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

This updates the typescript-fetch client with the latest dependencies based on work originally done in #4153 but in need of updates due to additional changes to @types/isomorphic-fetch that were causing build errors.

Additionally this adds the ability to upload files using the typescript-fetch client as requested in #3921. Given the limitations of the mustache template, I assumed that form parameters did not include files but, if so, it set a flag that caused the form parameters to be appended to a FormData object which is uploaded with a Content-Type: 'multipart/form-data' header opposed to the standard 'application/x-www-form-urlencoded'

Update type definitions for core-js and isomorphic-fetch

wing328 · 2017-03-05T16:25:50Z

@russtacular thanks for the PR.

@Vrolijkx @leonyu would you please take a look when you've time? Thanks.

Vrolijkx

looks great only one remark.

Vrolijkx · 2017-03-06T06:43:08Z

modules/swagger-codegen/src/main/resources/TypeScript-Fetch/package.json.mustache

@@ -6,16 +6,18 @@
  "browser": "./dist/api.js",
  "typings": "./dist/api.d.ts",
  "dependencies": {
-    {{^supportsES6}}"core-js": "^2.4.0",
-    {{/supportsES6}}"isomorphic-fetch": "^2.2.1"
+    "@types/node": "6.0.46",


Would be nice to also have this in the angular2 && angular clients.

I started looking at updating angular2 but I'm having issues building the copy in master using either TS 1.8.10 or my installed typescript@next developers build. I'm sure my unfamiliarity with building and using the angular generated libraries isn't helping either!

I can look into this but I think it may require more extensive changes and that it's likely better suited for a follow up PR. Is that okay with you?

Vrolijkx · 2017-03-06T06:59:31Z

modules/swagger-codegen/src/main/resources/TypeScript-Fetch/api.mustache

+        {{/isFile}}formData["{{baseName}}"] = params["{{paramName}}"];
+{{/formParams}}
+
+        if (isMultipartData) {


Why not instead of this if statement in all the generated code. Use a Mustache if. This way you don't need the next strange code:

let isMultipartData = false; isMultipartData = true;

maybe do something like this:

{{#isFile}} let body = new FormData(); for (let key in formData) { body.append(key, formData[key]); } fetchOptions.body = body; contentTypeHeader = { "Content-Type": "multipart/form-data" }; {{/isFile}} {{^isFile}} fetchOptions.body = querystring.stringify(formData); contentTypeHeader = { "Content-Type": "application/x-www-form-urlencoded" }; {{#isFile}}

I originally used a very similar implementation to the one you suggested and also mentioned in #3921 but noticed that, since {{#isFile}} is scoped to the individual parameter, it didn't work for requests that included a file and regular form parameters. I checked again and, unless I'm looping through the individual formParams, {{#isFile}} always maps to false.

Is there a Mustache template variable available that tracks if any of the included FormData attributes is of type file? If so, that would allow me to simplify the mustache template to essentially match your suggestion and get rid of the seemingly extraneous code.

Is there a Mustache template variable available that tracks if any of the included FormData attributes is of type file?

Not that I'm aware of. If it helps, we can introduce the variable.

russtacular · 2017-03-09T21:15:08Z

Hold off on this PR for a bit… I think it needs a small update and I'm also going to see about adding that form-level variable.

Notably I need to remove the explicit set of the content-type header, otherwise I'll make the same mistake that #3932 fixed.

…mically generated

Also removes references to typings in the Readme

Ensures that content-type is set for urlencoded forms but is specifically left unset for multipart form data since the content-type header value is dynamically generated to include a boundary.

…y-updates

…ile-upload-and-dependency-updates * swagger/master: (59 commits) [Java][Retrofit2] update gradle, sbt build file with latest dependencies (swagger-api#5238) update retrofit2 dependencies (swagger-api#5235) Issue swagger-api#3084: remove timestamps from undertow samples (swagger-api#5231) add port 80 to swagger spec (swagger-api#5232) fix msf4j bin script fix c# docstring typo (swagger-api#5223) add comments to csharp methods (swagger-api#5222) fix retrofit2 indentation (swagger-api#5221) Updating samples for spring-stubs, springboot-delegate, springboot-delegate-j8. (swagger-api#5211) export top level enumeration types in typescript-fetch mode (swagger-api#4820) [Erlang] pretty print swagger json (swagger-api#5215) add ci test for msf4j server (swagger-api#5220) Updating samples for JaxRS servers: Spec + CXF-CDI. (swagger-api#5213) Update samples for JaxRS/RestEasy/Joda. (swagger-api#5205) Update samples for Undertow. (swagger-api#5207) Update samples for okhttp-gson-parcelableModel. (swagger-api#5208) Update samples for Java Inflector. (swagger-api#5204) [java][msf4j] Update msf4j samples (and fix artifact name) (swagger-api#5210) Update samples for Ruby-Client + Rails server. (swagger-api#5214) [ASP.NET] Issue swagger-api#5196: add packageGuid parameter to AspNetCoreServerCodegen. (swagger-api#5199) ...

wing328 · 2017-04-24T14:27:02Z

@russtacular the CI (travis) failed with the following errors:


  StoreApiFactory
    without custom request options
      âœ“ should get inventory
    with custom request options
      âœ“ should get inventory


  26 passing (180ms)
  2 failing

  1) PetApi without custom request options should not set a content-type header when uploading a file:
     ReferenceError: File is not defined
      at Context.<anonymous> (dist/test/PetApi.js:49:120)

  2) PetApi with custom request options should not set a content-type header when uploading a file:
     ReferenceError: File is not defined
      at Context.<anonymous> (dist/test/PetApi.js:49:120)

Would you please take a look when you've time?

zetlan · 2017-05-17T18:27:25Z

Is there a timeline on this one? It would solve the same problem I'm having with file uploads via generated code.

russtacular · 2017-05-17T18:56:14Z

@wing328 @zetlan — Thanks for prodding me on this, I definitely still want to get this fix merged. The failing test, however, points out a bit of a problem with using isomorphic-fetch and trying to maintain compatibility with both browsers and Node.

Node doesn't support FormData which causes the node test to fail but the browser-based tests to pass. The options for fixing this are:

Use a shim to try and support FormData in a Node environment. This doesn't seem like a great option except for the fact that should maintain compatibility with any server-side usage but makes the TS Fetch client reliant on more niche and also not as actively maintained shim libraries.
Remove the expectation that TS Fetch work in Node environments unless they provide a FormData shim. That would keep client size down and we could conceivably still rely on packages like JSDom or similar to provide a FormData shim to enable Node-based tests to run.
Replace isomorphic-fetch with whatwg-fetch and break its ability to run in Node. Given that there's already a TS Node client, maybe this is fine, although it does kind of kill the dream of isomorphic type/javascript.

Those are the options I see, although I could definitely be missing something obvious. I happy to submit a fix that uses any of the options I identified above but I don't feel like I can make that decision unilaterally since I don't know how others are using this client (at Bitly, we're only using the generated TS Fetch client within browsers).

AmazingTurtle · 2018-01-15T21:05:51Z

Can someone have a look at this please? @russtacular

AmazingTurtle · 2018-01-16T09:04:35Z

samples/client/petstore/typescript-fetch/tests/default/test/PetApi.ts

+      });
+
+      it('should not set a content-type header when uploading a file', () => {
+        return new PetApi(parameterInspector).uploadFile({ petId: fixture.id, file: new File(['snoopy'], 'Snoopy.png')}).then(({arguments: [url, init]}) => {


File is not defined, that's why tests are failing

@AmazingTurtle good catch!

wing328 · 2018-01-20T06:21:43Z

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx to review and see how we can proceed with this PR.

Alexuy · 2018-06-27T16:41:19Z

Hey, guys! What about this issue? When you planning to fix that bug?

Place1 · 2018-06-27T23:13:16Z

Have a look at a fork I’ve made over at https://github.com/Place1/swagger-codegen-typescript-browser You can download a jar from the releases page It works with multipart form data requests and fixes a bunch of other bugs.

…

On Thu, 28 Jun 2018 at 2:41 am, Alexey Trofimov ***@***.***> wrote: Hey, guys! What about this issue? When you planning to fix that bug? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4852 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALMBllIbuIUxp9E2iFq8m-E-US6hNEWyks5uA7W9gaJpZM4MLqzD> .

russtacular and others added 3 commits February 24, 2017 15:26

Update TypeScript to v2.07+, update deps, remove typings

047debb

Update type definitions for core-js and isomorphic-fetch

Typescript-fetch: Send multipart data if file is detected

89c842b

Typescript-fetch: update petstore built files

9c2cb6c

wing328 added the Client: TypeScript label Feb 26, 2017

wing328 added this to the v2.2.3 milestone Feb 26, 2017

wing328 added the Enhancement: General label Feb 26, 2017

Vrolijkx approved these changes Mar 6, 2017

View reviewed changes

russtacular and others added 5 commits March 9, 2017 17:40

Don't set content-type header for multipart form-data since it's dyna…

86d7df2

…mically generated

Remove typings from typescript-fetch gitignore, samples, tests

76733ea

Also removes references to typings in the Readme

Add tests for content-type header

2bf37cb

Ensures that content-type is set for urlencoded forms but is specifically left unset for multipart form data since the content-type header value is dynamically generated to include a boundary.

Merge branch 'master' into typescript-fetch_file-upload-and-dependenc…

ab2f069

…y-updates

wing328 added Admin: 2.2.3 backlog and removed Admin: 2.2.3 backlog labels Jul 16, 2017

wing328 modified the milestones: v2.2.3, v2.3.0 Jul 16, 2017

Place1 mentioned this pull request Sep 5, 2017

[Typescript-fetch] url.URLSearchParams is not a constructor #6403

Closed

wing328 modified the milestones: v2.3.0, v2.4.0 Dec 18, 2017

AmazingTurtle suggested changes Jan 16, 2018

View reviewed changes

wing328 removed the Admin: 2.2.3 backlog label Jan 26, 2018

frantuma added the Language: typescript label Jun 27, 2018

jackjocross mentioned this pull request Feb 11, 2019

Add support for multipart/form-data when file is present stardogventures/swagger-codegen#1

Open

russtacular closed this Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescript-fetch: file upload and Typescript dependency updates #4852

Typescript-fetch: file upload and Typescript dependency updates #4852

russtacular commented Feb 24, 2017

wing328 commented Mar 5, 2017

Vrolijkx left a comment

Vrolijkx Mar 6, 2017

russtacular Mar 6, 2017

Vrolijkx Mar 7, 2017

Vrolijkx Mar 6, 2017

russtacular Mar 6, 2017

wing328 Mar 7, 2017

russtacular commented Mar 9, 2017 •

edited

Loading

wing328 commented Apr 24, 2017

zetlan commented May 17, 2017

russtacular commented May 17, 2017

AmazingTurtle commented Jan 15, 2018

AmazingTurtle Jan 16, 2018 •

edited

Loading

wing328 Jan 20, 2018

wing328 commented Jan 20, 2018

Alexuy commented Jun 27, 2018

Place1 commented Jun 27, 2018 via email

Typescript-fetch: file upload and Typescript dependency updates #4852

Typescript-fetch: file upload and Typescript dependency updates #4852

Conversation

russtacular commented Feb 24, 2017

PR checklist

Description of the PR

wing328 commented Mar 5, 2017

Vrolijkx left a comment

Choose a reason for hiding this comment

Vrolijkx Mar 6, 2017

Choose a reason for hiding this comment

russtacular Mar 6, 2017

Choose a reason for hiding this comment

Vrolijkx Mar 7, 2017

Choose a reason for hiding this comment

Vrolijkx Mar 6, 2017

Choose a reason for hiding this comment

russtacular Mar 6, 2017

Choose a reason for hiding this comment

wing328 Mar 7, 2017

Choose a reason for hiding this comment

russtacular commented Mar 9, 2017 • edited Loading

wing328 commented Apr 24, 2017

zetlan commented May 17, 2017

russtacular commented May 17, 2017

AmazingTurtle commented Jan 15, 2018

AmazingTurtle Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

wing328 Jan 20, 2018

Choose a reason for hiding this comment

wing328 commented Jan 20, 2018

Alexuy commented Jun 27, 2018

Place1 commented Jun 27, 2018 via email

russtacular commented Mar 9, 2017 •

edited

Loading

AmazingTurtle Jan 16, 2018 •

edited

Loading