Skip to content

[typescript-axios] Properly emit module as ES6 instead of commonJS when using ES6#10308

Merged
macjohnny merged 1 commit intoOpenAPITools:6.0.xfrom
ferferga:master
Sep 1, 2021
Merged

[typescript-axios] Properly emit module as ES6 instead of commonJS when using ES6#10308
macjohnny merged 1 commit intoOpenAPITools:6.0.xfrom
ferferga:master

Conversation

@ferferga
Copy link
Copy Markdown
Contributor

@ferferga ferferga commented Sep 1, 2021

When using ES6 in the generator, the generated sources are indeed ES6. However, when building the package (and publishing to npm or other registries, as that's done automatically), TypeScript compiler builds a commonJS version of the code, which goes against the desire of the user's desire to build ES6.

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.

@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)

@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Sep 1, 2021

Thanks, looks good, but since it is a breaking change, please file against the 6.0 branch

@ferferga ferferga changed the base branch from master to 6.0.x September 1, 2021 18:01
@ferferga
Copy link
Copy Markdown
Contributor Author

ferferga commented Sep 1, 2021

@macjohnny Done! 👍

macjohnny
macjohnny approved these changes Sep 1, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@macjohnny macjohnny merged commit a447df0 into OpenAPITools:6.0.x Sep 1, 2021
@wing328
Copy link
Copy Markdown
Member

wing328 commented Sep 2, 2021

Travis CI failed with the following:

Java version: 1.8.0_252, vendor: Private Build, runtime: /usr/lib/jvm/java-8-openjdk-amd64/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "4.15.0-1077-gcp", arch: "amd64", family: "unix"
npm notice Beginning October 4, 2021, all connections to the npm registry - including for package installation - must use TLS 1.2 or higher. You are currently using plaintext http to connect. Please visit the GitHub blog for more information: https://github.blog/2021-08-23-npm-registry-deprecating-tls-1-0-tls-1-1/

added 4 packages, and audited 5 packages in 1s

1 package is looking for funding
  run `npm fund` for details

found 0 vulnerabilities
npm notice Beginning October 4, 2021, all connections to the npm registry - including for package installation - must use TLS 1.2 or higher. You are currently using plaintext http to connect. Please visit the GitHub blog for more information: https://github.blog/2021-08-23-npm-registry-deprecating-tls-1-0-tls-1-1/

> [email protected] preinstall
> npm install ../../builds/with-npm-version && npm run build

npm notice Beginning October 4, 2021, all connections to the npm registry - including for package installation - must use TLS 1.2 or higher. You are currently using plaintext http to connect. Please visit the GitHub blog for more information: https://github.blog/2021-08-23-npm-registry-deprecating-tls-1-0-tls-1-1/

up to date, audited 277 packages in 767ms

3 moderate severity vulnerabilities

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> [email protected] build
> tsc

tsconfig.json(4,19): error TS6046: Argument for '--module' option must be: 'none', 'commonjs', 'amd', 'system', 'umd', 'es6', 'es2015', 'es2020', 'esnext'.
npm ERR! code 2
npm ERR! path /home/travis/build/OpenAPITools/openapi-generator/samples/client/petstore/typescript-axios/tests/default
npm ERR! command failed
npm ERR! command sh -c npm install ../../builds/with-npm-version && npm run build

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/travis/.npm/_logs/2021-09-02T02_48_32_487Z-debug.log
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (npm-install) on project TypeScriptAxiosPestoreClientTests: Command execution failed. Process exited with an error: 2 (Exit value: 2) -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (npm-install) on project TypeScriptAxiosPestoreClientTests: Command execution failed.
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:215)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:156)

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

Ref: https://app.travis-ci.com/github/OpenAPITools/openapi-generator/builds/236742100

@ferferga
Copy link
Copy Markdown
Contributor Author

ferferga commented Sep 2, 2021

@wing328 Seems it doesn't like uppercase names even when Typescript own documentation allows it

The generated packages should still work, so anyone using the beta or compiling should not have any problems. I have those keys always upper-cased in all my projects without issues. So this seems more like a linting problem.

Will make a PR for this later.

@wing328
Copy link
Copy Markdown
Member

wing328 commented Sep 2, 2021

What version of typescript are you using?

@ferferga
Copy link
Copy Markdown
Contributor Author

ferferga commented Sep 2, 2021

@wing328 4.2.4

@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Sep 2, 2021

I think it is about the „module“ for non-es6, which should remain commonjs. @ferferga can you file a PR to fix that?

@ferferga ferferga mentioned this pull request Sep 2, 2021
Merged
5 tasks
@ferferga
Copy link
Copy Markdown
Contributor Author

ferferga commented Sep 2, 2021

@macjohnny Totally true! Read the log in my mobile phone quickly while commuting and thought it was all complaing about capitalization, but it's the CommonJS thing!

PR up at #10316 as you can see linked above in this thread (this time pointing to the correct branch and with fully passing tests! :D)

CC @wing328 as well.

@wing328 wing328 added this to the 5.3.0 milestone Oct 23, 2021
@ferferga ferferga mentioned this pull request Nov 22, 2021
Closed
@ferferga ferferga mentioned this pull request Nov 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@macjohnny macjohnny macjohnny approved these changes

Assignees

No one assigned

Labels

Breaking change (without fallback) Client: TypeScript

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@ferferga @macjohnny @wing328