Skip to content

Conversation

@hmt
Copy link

@hmt hmt commented Feb 7, 2021

In this PR I changed the namespaced import of url-parse to default import. The namespaced import caused all kinds of trouble with my rollup configuration and it seems to work either way. I might be wrong but to my knowledge TS doesn't need namespaced imports anymore. Let me know if this is totally off and should be solved differently.

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.1.x, 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.
    Thank you for looking into this: @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)

hmt and others added 2 commits February 6, 2021 09:37
Using the star import makes rollup barf all over the place because it doesn't know url-parse is a cjs module.
@TiFu
Copy link
Contributor

TiFu commented Feb 7, 2021

Thank you for the PR! Looks like there is some issue with the way the default import works versus what we had before - see the CI output below. Is it possible that we would have to call a function of URLParse or remove the new keyword?

 1) PetApi
       addPet:
     TypeError: url_parse_1.default is not a constructor
      at new RequestContext (/home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/http/http.ts:60:20)
      at ServerConfiguration.makeRequestContext (/home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/servers.ts:47:10)
      at PetApiRequestFactory.<anonymous> (/home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/apis/PetApi.ts:35:47)
      at step (/home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:46:23)
      at Object.next (/home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:27:53)
      at /home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:21:71
      at new Promise (<anonymous>)
      at __awaiter (/home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:17:12)
      at PetApiRequestFactory.addPet (/home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:65:16)
      at ObservablePetApi.addPet (/home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/types/ObservableAPI.ts:35:56)
      at PromisePetApi.addPet (/home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/types/PromiseAPI.ts:31:30)
      at Context.<anonymous> (test/api/PetApi.test.ts:23:16)
      at processImmediate (internal/timers.js:461:21)

@hmt
Copy link
Author

hmt commented Feb 7, 2021

I'm not entirely sure what is going on but I'm using es6 in my tsconfig via the es6 flag unlike the tests that use es5 and commonjs. Should there be another build for es6 only?

@TiFu
Copy link
Contributor

TiFu commented Feb 7, 2021

We could do a separate build for es6 - either way the code would have to work for both es5 and es6.

Could you elaborate a bit more on what trouble you ran into with the namespaced import?

@hmt
Copy link
Author

hmt commented Feb 7, 2021

I'm using vite for an app. As I understand it it uses different build systems for development and production. In dev mode it works fine but in production mode it switches to rollup.

So when I use rollup to build the code I end up with this error message:

Uncaught (in promise) TypeError: URLParse is not a constructor
    RequestContext http.js:35
    makeRequestContext servers.js:22
    getSchuelerListe SchuelerApi.js:60
    1 SchuelerApi.js:7
    1 SchuelerApi.js:3
    getSchuelerListe SchuelerApi.js:53
    getSchuelerListe ObservableAPI.js:1854
    getSchuelerListe ObjectParamAPI.js:424
    <anonymous> main.js:6
http.js:35:19

@TiFu
Copy link
Contributor

TiFu commented Feb 7, 2021

Mhh, to me that sounds like an issue with rollout - or an incompatibility between ES5 and 6.

My way forward would either be raising this with rollout and getting their opinion on this issue - or looking at what the difference in generated code is between dev and prod. That should at least give a hint of what's wrong and from there may be why it is happening.

@wing328
Copy link
Member

wing328 commented Feb 10, 2021

We could do a separate build for es6 - either way the code would have to work for both es5 and es6.

Shall we consider dropping es5 support in the TS generators?

We've already done it in the javascript generator as part of the 5.0.0 release.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants