Skip to content

[typescript] Make module usable with esbuild #11298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 13, 2022

Conversation

bodograumann
Copy link
Contributor

When trying to use the current output code from the typescript generator with esbuild (or vite to be more specific) the import from url-parse is not usable. This template-only change works around the problem.
I was also able to use typings for url-parse.

  • Use default import from url-parse
  • Update samples
  • Fix typo in readme

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)

@bodograumann
Copy link
Contributor Author

Test failures in pipeline seem unrelated.

@macjohnny macjohnny merged commit 7129cde into OpenAPITools:master Jan 13, 2022
@wing328
Copy link
Member

wing328 commented Jan 16, 2022

Looks like this breaks the CircleCI build:

1) PetApi
       "before each" hook for "addPet":
     TypeError: url_parse_1.default is not a constructor
      at new RequestContext (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/http/http.ts:59:20)
      at ServerConfiguration.makeRequestContext (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/servers.ts:47:16)
      at PetApiRequestFactory.<anonymous> (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/apis/PetApi.ts:37:51)
      at step (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:46:23)
      at Object.next (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:27:53)
      at /home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:21:71
      at new Promise (<anonymous>)
      at __awaiter (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:17:12)
      at PetApiRequestFactory.addPet (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:66:16)
      at ObservablePetApi.addPet (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/types/ObservableAPI.ts:34:59)
      at PromisePetApi.addPet (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/types/PromiseAPI.ts:30:33)
      at Object.<anonymous> (test/api/PetApi.test.ts:25:22)
      at Generator.next (<anonymous>)
      at /home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/tests/default/test/api/PetApi.test.ts:7:71
      at new Promise (<anonymous>)
      at __awaiter (test/api/PetApi.test.ts:3:12)
      at Context.<anonymous> (test/api/PetApi.test.ts:16:27)
      at processImmediate (node:internal/timers:464:21)

  2) Security Authentication
       API Key Authentication
         Header API Key:
     TypeError: url_parse_1.default is not a constructor
      at new RequestContext (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/http/http.ts:59:20)
      at Context.<anonymous> (test/auth/auth.test.ts:10:23)
      at processImmediate (node:internal/timers:464:21)



[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (npm-test) on project TypeScriptDefaultPetstoreClientTests: Command execution failed. Process exited with an error: 2 (Exit value: 2) -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:

@macjohnny
Copy link
Member

@bodograumann can you fix this? Or should we revert?

@bodograumann
Copy link
Contributor Author

Yes. Sorry that I missed this.
I realize now that the proposed solution would only work with babel, because allowSyntheticDefaultImports does not actually change the generated code :-(

For now it is probably better to revert this PR. 🤕 🧱
I’ll have to analyze the issue more deeply.

@macjohnny
Copy link
Member

Reverted

@amakhrov
Copy link
Contributor

amakhrov commented Jan 17, 2022

@bodograumann
I believe allowSyntheticDefaultImports should be used together with esModuleInterop - so that the generated code actually contains those synthetic default imports.

Babel has this setting enabled by default. But tsc - does not. Not sure about esbuild - perhaps it always acts as if this setting is permanently enabled

@bodograumann
Copy link
Contributor Author

Thanks for the hint @amakhrov .
I was actually getting a warning from esbuild, that the generated library for browsers is neither really commonjs nor a proper esmodule. Maybe the fact that we are using parse-url as a commonjs dependency is a problem.

@bodograumann
Copy link
Contributor Author

I’m trying to add some proper tests for esmodules in browser, but cannot get them to run.
The browser always forces https for petstore.swagger.io and this does not work with the swaggerapi/petstore docker container.
Has anybody come across this problem before?

Cf. https://github.com/bodograumann/openapi-generator/tree/ts-url-parse-import

@amakhrov
Copy link
Contributor

As to why https is enforced - it's HSTS preloaded: https://hstspreload.org/?domain=petstore.swagger.io

Surprised that using https website with a good certificate cause any troubles. Any details on the issue you're seeing?

@bodograumann
Copy link
Contributor Author

We don’t actually run the integration tests against the real petstore.swagger.io instance, @amakhrov. Doing so would cause various issues. E.g. there are multiple instances behind that domain and on each request you get a random one. So if you create a pet with an id, it might be unknown on the next request.
Locally, to run integration tests, the petstore server is started as docker container and hooked in via an /etc/hosts entry for petstore.swagger.io. I assume in the pipeline something similar is happening. See the wiki for documentation.

This is no problem at all when running outside the browser. However, as you correctly noticed, the browser uses HSTS and there are also other security features that cause problems in the browser. E.g. I had to disable CORS. It just seems like there is no way to disable the preloaded HSTS domains. 😐

@amakhrov
Copy link
Contributor

oh, ok! is there a reason not to change the url in the test spec from petstore.swagger.io to something else (e.g. example.com), since we anyway point it to the local container with hosts.

@bodograumann
Copy link
Contributor Author

Given that the server actually runs on localhost:80 that is definitely something we can use locally. Looks like .circleci/config.yml also runs the petstore server the same way. So hopefully it will also work in the pipeline.

It looks quite good so far. Let me create a new pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants