Skip to content
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

Fix NPM build for Typescript-fetch #3403

Merged
merged 6 commits into from
Aug 6, 2019
Merged

Conversation

JFCote
Copy link
Member

@JFCote JFCote commented Jul 19, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10) @akehir (2019/07)

Description of the PR

This PR fixes 2 things:

  1. The npm build was "working" but was not usable as a package because there was no output generated (Fix by adding the .npmignore file). Because of this, when you used the package via npm, webpack was always telling you that you need a specific loader for ts files. A real package should contains compiled code (.js).
  2. I followed a tutorial on how to do a good typescript package so I followed convention of putting "source" code in the "src" folder instead of directly at the root.

I followed this tutorial to do a good npm package: https://medium.com/cameron-nokes/the-30-second-guide-to-publishing-a-typescript-package-to-npm-89d93ff7bccd

-Move fetch files to an "src" folder instead of doing everything at the root which removes the bugs when using the library itself
-Fix the distribution of fetch using npm
# Conflicts:
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptFetchClientCodegen.java
#	samples/client/petstore/typescript-fetch/builds/multiple-parameters/models/Category.ts
#	samples/client/petstore/typescript-fetch/builds/multiple-parameters/models/ModelApiResponse.ts
#	samples/client/petstore/typescript-fetch/builds/multiple-parameters/models/Tag.ts
Copy link
Contributor

@TiFu TiFu left a comment

Choose a reason for hiding this comment

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

LGTM if the circleci failure is unrelated

@macjohnny
Copy link
Member

macjohnny commented Jul 23, 2019

@JFCote can you please add the .npmignore files to the samples?

Untracked files:
(use "git add ..." to include in what will be committed)
samples/client/petstore/typescript-fetch/builds/es6-target/.npmignore
samples/client/petstore/typescript-fetch/builds/with-npm-version/.npmignore

@JFCote
Copy link
Member Author

JFCote commented Jul 28, 2019

@macjohnny Currently in vacation, will check all of this when I'm back! Thanks for the review!

@macjohnny macjohnny merged commit 9e659d1 into master Aug 6, 2019
@macjohnny macjohnny deleted the fix-to-typescript-fetch branch August 6, 2019 14:08
@wing328 wing328 added this to the 4.1.0 milestone Aug 9, 2019
jimschubert added a commit that referenced this pull request Aug 11, 2019
* master: (122 commits)
  Fix #3604 by adding undefined as return type to headers and credentials methods in runtime.ts (#3605)
  Prepare 4.1.1-SNAPSHOT (#3603)
  Prepare 4.1.0 release (#3597)
  [java][client][jax-rs] Add a constant for Jackson @JsonProperty (#3560)
  restore openapi3 petstore.yaml (#3590)
  Add a new NodeJS Express server generator (#3567)
  [C#][client][csharp-netcore] Fix csharp netcore defaultheaders (#3562)
  Fix issue deserializing to nullptr (#3572)
  [OCaml] Add file post-processing (#3583)
  [dart2] Fix up test code generation, double deserialization, 'new' keyword deprecation (#3576)
  Run Qt5 client sample test (#3415)
  typescript-fetch: allow configuration of headers and credentials (#3586)
  using partials in ruby api_client (#3564)
  [OCaml] Added optional params support in API operations (#3568)
  [Rust Server] Generate valid Rustdocs for lazy_static items (#3556)
  Fix NPM build for Typescript-fetch (#3403)
  Expand path templates via resttemplate's uriTemplateHandler (#3500)
  Readme updated with a new tutorial and company using OpenAPI Generator (#3566)
  Fix logic of `getNullableType` of csharp server and client. (#3537)
  [Ruby] clean up Ruby dev dependencies (#3551)
  ...
@Bessonov
Copy link

@JFCote

I followed a tutorial on how to do a good typescript package so I followed convention of putting "source" code in the "src" folder instead of directly at the root.

Although we use src folder by default, this is a breaking change for us.

@JFCote
Copy link
Member Author

JFCote commented Sep 24, 2019

@Bessonov Can you explain a little bit more how this is a breaking change? If you use npm publish, you should'nt have any problem importing the resulting package, even if the structure has changed because webpack take care of everything.

Let us know what is your use case. Thanks

@Bessonov
Copy link

Bessonov commented Sep 24, 2019

We don't use it as a whole module, but integrate it in own api module with some utilities and configuration. Basically, we have following structure:

- myApi
  - src
    - index.ts <-- exports only relevant and configured parts
    - configuration.ts
    - fetch <-- here we place the generated artifacts

After this change the artifacts are not under src/fetch, but src/fetch/src. Of course we can handle it with some post processing, but well, that's a definition of breaking changes :)

@JFCote
Copy link
Member Author

JFCote commented Sep 24, 2019

@Bessonov I understand now. The changes was supposed to be mostly for the npm version of the generator but it seems it has created some regression for use case like yours.

@macjohnny What do you suggest we do for this?

@Bessonov
Copy link

@JFCote no problem. I've implemented a postprocessing hook now, so don't care about us 👍

@rob-deutsch
Copy link
Contributor

@Bessonov what postprocessing did you implement for this?

Like you, I was also using openapi-generator to integrate it into an existing npm package.

@Bessonov
Copy link

Bessonov commented Nov 8, 2019

@rob-deutsch because all generators has some flaws, we use a run scripts like (pseudo code):

#!/bin/bash

# pre processing
rm -rf ...

# do work
docker run generator ....

# post processing
# remove files
for path in ".gitignore" ".openapi-generator-ignore" "tsconfig.json" ".openapi-generator"; do
	rm -rf "${rest_api}/src/fetch/${path}"
done
# new version of generator create resources in src folder
mv ${rest_api}/src/fetch/src/* ${rest_api}/src/fetch
rmdir ${rest_api}/src/fetch/src

So, no magic.

@rob-deutsch
Copy link
Contributor

Thanks @Bessonov. That's more elegant than my import * from './service/src'; solution

@rob-deutsch
Copy link
Contributor

rob-deutsch commented Nov 12, 2019

[edit]: Disregard this post. I was running into issues with the additional tsconfig.json file.

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.

None yet

6 participants