[typescript-axios] Add option to add NodeJS imports by vesse · Pull Request #10990 · OpenAPITools/openapi-generator

vesse · 2021-11-29T16:50:06Z

While NodeJS has had global URL and URLSearchParams for quite some time they still need to be imported when using TypeScript and not adding DOM to libs (see DefinitelyTyped/DefinitelyTyped#34960). Since server side code should not really need DOM added an option to generate the imports needed so there is no need to postprocess the generated code.

Also add import for form-data when using both withNodeImports and multipartFormData, and add the form data headers to localVarRequestOptions.headers if FormData object has function getHeaders.

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02)

* Added new option `withImportUrl` to be used to generate the imports needed for NodeJS support without adding DOM to TypeScript libs

* Generate imports from 'url' if withImportUrl is set to true

…old param name

arminrosu · 2021-11-29T17:00:25Z

 |sortParamsByRequiredFlag|Sort method arguments to place required parameters before optional parameters.| |true|
 |supportsES6|Generate code that conforms to ES6.| |false|
 |useSingleRequestParameter|Setting this property to true will generate functions with a single argument containing all API endpoint parameters instead of one argument per parameter.| |false|
+|withImportUrl|Setting this property to true adds imports from 'url' to the generated code for NodeJS support| |false|


Thanks, came here for the same issue and you already have a PR. Thanks!

I would suggest though to rename this option to withNodeImports and add an import FormData from 'form-data' to api.mustache as well.

This is required for multipart/formData file uploads:

openapi-generator/modules/openapi-generator/src/main/resources/typescript-axios/apiInner.mustache

Line 65 in f27799c

const localVarFormParams = new {{^multipartFormData}}URLSearchParams(){{/multipartFormData}}{{#multipartFormData}}((configuration && configuration.formDataCtor) || FormData)(){{/multipartFormData}};{{/hasFormParams}}{{/vendorExtensions}}

Sure, makes sense. Didn't notice this need because I haven't used form-data myself. Just a moment.

Sorry to bother again, I just checked my code, and there is 1 modification more I had to make re formData:

openapi-generator/modules/openapi-generator/src/main/resources/typescript-axios/apiInner.mustache

Line 192 in f27799c

localVarRequestOptions.headers = {...localVarHeaderParameter, ...headersFromBaseOptions, ...options.headers};

- localVarRequestOptions.headers = {...localVarHeaderParameter, ...headersFromBaseOptions, ...options.headers}; + localVarRequestOptions.headers = {...localVarHeaderParameter, ...headersFromBaseOptions, ...localVarFormParams.getHeaders(), ...options.headers};

It seems otherwise it doesn't add the necessary length headers. And because the function it generates can take a stream as input, it might be difficult / inconvenient to set the length as a custom options.headers

@arminrosu now added also form-data import and the getHeaders call if the FormData object at hand has the method (browser FormData does not have the function so this change should not break browser usage)

arminrosu · 2021-11-29T17:02:34Z

Thanks again Vesa!

@ maintainers - if this PR gets merged, I suggest you remove the typescript-node generator, as it uses request internally, which has been deprecated for years.

* Rename the parameter to support other Node imports * Add imports for form-data too if using multipartFormData * Add fix for multipart headers when running in Node with form-data package

vesse · 2021-11-29T18:37:44Z

+// @ts-ignore
+import { URL, URLSearchParams } from 'url';
+{{#multipartFormData}}
+import FormData from 'form-data'


Not sure if this should have // @ts-ignore – it is of course possible to set the multipartFormData parameter for a spec that does not have any form data endpoints and then this import would be unused (and probably the form-data dependency is missing too if not needed), but then again is it a user error if they configure to use something that is not needed?

is it a user error if they configure to use something that is not needed?

In my view, it totally is. Or otherwise put - if you declared your need for this code but it's not there (e.g. you want to manually extend the file), would that not be a generation error?

In my humble opinion, neither of eslint-disable, tslint:disable or ts-ignore should be present in this file. All of the unused imports can and should be fixed using eslint --fix, prettier --write or whatever code formatter someone uses. Since this is a programatic change, not a manual one, you can consistently apply it after each generation.

That's how I do it. I remove all these comments using sed after generating the source code and before formatting. The only warnings I get are related to the use of any typings.

Besides, tslint has also been deprecated since 2019.

Btw, typescript-node has quite a few linting errors, including circular dependencies - yet another reason to deprecate that generator template.

eslint --fix doesn't fix unused imports
and TS itself doesn't have autofix capability (so noUnusedLocals compiler setting will simply fail to compile the generated code)

@amakhrov I'm using eslint-plugin-unused-imports to remove unused imports.

IMO, the generated code should either be valid, or not disable linting. The latter is especially tricky, since you must support all linting tooling (e.g. there should also be a .prettierignore), which to me seems unfeasable.

What other code generators do, is lint and/or format the code themselves after generation. That could be an option for JS as well.

But we are diverging from the topic of the PR.

vesse · 2021-12-14T07:58:23Z

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02)

Anything I could add / concerns?

vesse · 2021-12-15T09:07:18Z

OK so while waiting we decided to jump to Node 16 and with newer @types/node this would not be needed anymore, so kinda mixed feelings if this needs to be supported or not.

arminrosu · 2021-12-15T10:13:08Z

we decided to jump to Node 16 and with newer @types/node this would not be needed anymore, so kinda mixed feelings if this needs to be supported or not.

@vesse not sure that works for everyone. We're already on @types/[email protected] but we still expect explicit imports in our codebase (for various readability and tooling reasons).

While you don't need this anymore, we most certainly do and appreciate the PR all the much more 🤗 .

AaronElijah · 2021-12-16T13:13:07Z

+1 on this -> my company use NextJS for almost all of our web apps, which requires node >12.22.0 so the PR is very much appreciated! Thank you! 👍

vesse · 2022-01-03T18:20:39Z

@macjohnny thanks! I don't know for what you use the milestones but this was not included in 5.3.1.

macjohnny · 2022-01-03T20:49:00Z

Thanks, updated the milestone to 5.4.0

vesse added 5 commits November 29, 2021 18:37

[typescript-axios] Add new option to generate imports from 'url'

243efed

* Added new option `withImportUrl` to be used to generate the imports needed for NodeJS support without adding DOM to TypeScript libs

[typescript-axios] Add withImportUrl support to templates

8529b5f

* Generate imports from 'url' if withImportUrl is set to true

[typescript-axios] Generate new samples using withImportUrl

5e5ff8e

[typescript-axios] Add withImportUrl to documentation

c64ced2

[typescript-axios] Regenerate docs, build was still ongoing and used …

f27799c

…old param name

arminrosu reviewed Nov 29, 2021

View reviewed changes

[typescript-axios] Rename withImportUrl to withNodeImports

f7044e2

* Rename the parameter to support other Node imports * Add imports for form-data too if using multipartFormData * Add fix for multipart headers when running in Node with form-data package

vesse changed the title ~~[typescript-axios] Add option to add imports from 'url' to generated code~~ [typescript-axios] Add option to add NodeJS imports Nov 29, 2021

vesse commented Nov 29, 2021

View reviewed changes

macjohnny merged commit 7ffd071 into OpenAPITools:master Dec 24, 2021

macjohnny added the Client: TypeScript label Dec 24, 2021

macjohnny added this to the 5.3.1 milestone Dec 24, 2021

macjohnny modified the milestones: 5.3.1, 5.4.0 Jan 3, 2022

vesse deleted the feature/typescript-axios-add-import-url-option branch January 3, 2022 21:25

Uh oh!

Conversation

vesse commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminrosu commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vesse commented Dec 14, 2021

Uh oh!

vesse commented Dec 15, 2021

Uh oh!

arminrosu commented Dec 15, 2021

Uh oh!

AaronElijah commented Dec 16, 2021

Uh oh!

vesse commented Jan 3, 2022

Uh oh!

macjohnny commented Jan 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vesse commented Nov 29, 2021 •

edited

Loading

arminrosu commented Nov 29, 2021 •

edited

Loading