Skip to content

Typescript refactor: stub rxjs #4424

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 9 commits into from
Nov 19, 2019
Merged

Typescript refactor: stub rxjs #4424

merged 9 commits into from
Nov 19, 2019

Conversation

bodograumann
Copy link
Contributor

As discussed in #802 I have replaced RxJS with a stub when it is not really needed.
There is a new option for the typescript generator useRxJS which defaults to false.

Additionally I fixed some minor issues.

I noticed that the tests in samples/client/petstore/typescript/tests are not automatically generated, but statically committed. So that is another open todo?

CC @TiFu

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.

Thanks for the PR!

Just a couple small questions:

Did you compile it locally and did it work? The issue below looks unrelated to your changes - I am wondering if that was an issue previously because I remember it being okay in your previous PR.

> [email protected] build /home/travis/build/OpenAPITools/openapi-generator/samples/client/petstore/typescript/builds/default

> tsc

http/http.ts(29,8): error TS2304: Cannot find name 'Buffer'.
http/http.ts(29,8): error TS4033: Property 'data' of exported interface has or is using private name 'Buffer'.

node_modules/form-data/index.d.ts(6,23): error TS2688: Cannot find type definition file for 'node'.
node_modules/form-data/index.d.ts(7,25): error TS2307: Cannot find module 'stream'.
node_modules/form-data/index.d.ts(8,23): error TS2307: Cannot find module 'http'.
node_modules/form-data/index.d.ts(28,16): error TS2304: Cannot find name 'Buffer'.

Also looks like there are uncommitted changes to the generator README (i.e. ensure-up-to-date.sh wasn't executed)

UNCOMMITTED CHANGES ERROR
There are uncommitted changes in working tree after execution of 'bin/ensure-up-to-date'
Perform git diff
diff --git a/docs/generators/typescript.md b/docs/generators/typescript.md
index e5d3540d8a..11ebf4ce29 100644
--- a/docs/generators/typescript.md
+++ b/docs/generators/typescript.md
@@ -12,4 +12,5 @@ sidebar_label: typescript
 |modelPropertyNaming|Naming convention for the property: 'camelCase', 'PascalCase', 'snake_case' and 'original', which keeps the original name| |camelCase|
 |supportsES6|Generate code that conforms to ES6.| |false|
 |fileContentDataType|Specifies the type to use for the content of a file - i.e. Blob (Browser) / Buffer (node)| |Buffer|
+|useRxJS|Enable this to internally use rxjs observables. If disabled, a stub is used instead. This is required for the 'angular' framework.| |false|
 |framework|Specify the framework which should be used in the client code.|<dl><dt>**fetch-api**</dt><dd>fetch-api</dd><dt>**jquery**</dt><dd>jquery</dd><dl>|fetch-api|
Perform git status
On branch pull/4424
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   docs/generators/typescript.md

no changes added to commit (use "git add" and/or "git commit -a")
Please run 'bin/utils/ensure-up-to-date' locally and commit changes (UNCOMMITTED CHANGES ERROR)

@@ -55,12 +55,12 @@ export interface PromiseMiddleware {
* @returns an observable of the updated request context
*
*/
pre(context: RequestContext): Observable<RequestContext>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed to Promise?

@bodograumann
Copy link
Contributor Author

bodograumann commented Nov 10, 2019

Compilation errors

I compiled locally. The mvn and generating the samples worked fine.
The tests did not work. I am not sure why that is. It was indeed confusing because the last time it worked, but when I checked out the typescript-refactor branch and rebuild everything, they still would not work. I guess I need to take a little more time to investigate this issue.

Readme

Yeah, I did not run ensure-up-to-date.sh again. I was hoping it wasn't needed because last time it changed unrelated things differently than the CI.
Have added the docs line for the useRxJS switch manually now.

Middleware

I changed it to what I think the original intention was. This is the only way that makes sense to me. Previously both interfaces Middleware and PromiseMiddleware where identical.
When thinking about how middlewares should work with RxJS stubed, I came to the conclusion, that only PromiseMiddleware which is then wrapped, can be used. The user should not be aware of the Observable stub, so he must work only with Promise.

I updated the code comments to make everything a bit clearer, I hope.

@bodograumann
Copy link
Contributor Author

Ok, got the tests to run on the original branch and found the problem:
We require the node types, but only implicitly depended on them via @types/form-data.
I have now added them as an explicit dependency.
Furthermore I have adjusted the dependencies in the tests according to the changes to the template and samples.

For the jquery tests to run I had to install webpack.

The tests where using Observable.subscribe. Afaics this is currently not really necessary, so I replaced it with the stubbed Observable.toPromise().then. If in the future it is needed, we can of course add it to the stub.

Not sure whether the jquery tests ran properly on my local machine, because I got

> node ./dist/test-runner.js

[error] Failed to load resource: the server responded with a status of 404 (Not Found)

But let's see what the ci says.

@bodograumann
Copy link
Contributor Author

The failures don’t seem to be related to my changes.
What do you think, @TiFu?

@TiFu
Copy link
Contributor

TiFu commented Nov 13, 2019

Thanks a lot for investigating this issue and the PR!

  • CircleCI is unrelated
  • DroneIO as well
    Going to close and reopen this PR to rerun the tests. If it fails again, i'll just merge anyway.

Compilation errors
Readme

Sounds good

Middleware

Oh the change was in PromiseMiddleware. Good catch.

@TiFu TiFu closed this Nov 13, 2019
@TiFu TiFu reopened this Nov 13, 2019
@bodograumann
Copy link
Contributor Author

Ok, forgot to remove subscribe from the jquery tests. Now it really is complete :-D
Tests seem good. Failure because of some OCAML stuff. Different from before but still unrelated to these changes.

@TiFu TiFu merged commit 6dfe637 into OpenAPITools:typescript-refactor Nov 19, 2019
@TiFu
Copy link
Contributor

TiFu commented Nov 19, 2019

Great! Again, thanks for the PR which has been merged into ts-refactor.

@wing328 wing328 added this to the 4.2.2 milestone Dec 2, 2019
@wing328
Copy link
Member

wing328 commented Dec 2, 2019

@bodograumann thanks for the PR, which has been included in the v4.2.2 release: https://twitter.com/oas_generator/status/1201432648544972800

@bodograumann bodograumann deleted the typescript-refactor-stub-rxjs branch March 4, 2020 13:08
TiFu pushed a commit that referenced this pull request Apr 26, 2020
* Remove unused supportsES6 field from codegen

* Add a new switch for RXJS

* Remove redundant npm dependency on rxjs4 types

* Fix return type of PromiseMiddleware methods

* Install webpack dependency to run jquery tests

* Update form-data to 2.5 which includes typings

* Add missing dependency on node typings

* Fix test artifact name typo

* Stub rxjs when it is not explicitly enabled
macjohnny added a commit that referenced this pull request Jun 15, 2020
… jquery (#6341)

* Added http module draft

* Added generic enum

* Modified http lib, added config & middleware definition to ts-fetch

* Added model generation with imports

* Added auth module

* Added servers

* Added sample for typescript client

* WIP: Models & API

* Updated auth

* WIP: api modeling

* Implemented RequestFactory and Processor completely

* Implemented fetch client

* Ignore dist folder in typescript client sample

* Added middleware to fetch

* Restructured TypeScript generator

* Reverted: http library.send returns string again

* Removed TODOs

* Added pom.xml files to TypeScript PetStore client samples

* Removed tabs from TypeScriptClientCodegen

* Added ts client codegen to root pom.xml and travis

* Added server variable configuration to ts-refactor

* [TS-Refactor] Added tests for Object Serializer

* Added simple test for PetApi

* Fixed ObjectSerializer test

* Added handling for different http status codes and test for deletePet

* Removed tabs in TypeScriptClientCodegen

* Removed tabs in DefaultCodegen

* Additional tests for pet store api

* Fixed file uploads

* Made api call configuration separately settable

* Use string union for enums

* Remove tab

* Restructured module layout

* Use observables internally

* Added promise based middleware

* Made discriminator and attributeTypeMap readonly

* Configure discriminator correctly

* Set discriminator value automatically

* Fixed date-time and date handling

* Added comments & license info

* Added comments

* Ignore openapi-generator-cli/bin

* Removed accidentally created generated code

* Fixed compilation issues in TypeScriptClientCodegen

* Added typescript to docs/generators

* Updated docs

* Added gitignore and git_push

* Added jquery library

* Added pom.xmls, fixed packagejsons and hopefully webppack

* Removed tabs in TypeScriptClientCodegen

* Fixed a couple issues with pom.xml

* Ensured up to date

* Fixed missing fetch definition in TS default tests

* Updated typescript docs

* Refactor typescript merge master (#4319)

Merge master into ts-refactor

* Typescript refactor: stub rxjs (#4424)

* Remove unused supportsES6 field from codegen

* Add a new switch for RXJS

* Remove redundant npm dependency on rxjs4 types

* Fix return type of PromiseMiddleware methods

* Install webpack dependency to run jquery tests

* Update form-data to 2.5 which includes typings

* Add missing dependency on node typings

* Fix test artifact name typo

* Stub rxjs when it is not explicitly enabled

* Typescript refactor: Platform select for browser and node (#4500)

* Use string form of filename parameter

This works for the form-data library and is also compatible with the
browser FormData object.

* Add new option to select platform node or browser

When no platform is selected, a default is chosen by the framework
option and likewise the file data type option is implied by the
platform.

* Remove redundant import of node dns module

* Only use form-data library for node platform

* Generate npm package from npmName option

* Use method convertPropertyToBooleanAndWriteBack

* Generate typescript samples with ensure-up-to-date

* Removed tab from DefaultCodegen

* Readded missing change

* Mark typescript client codegen as experimental

* Removed whitespace

* [TS-Refactor] Top-level exports for fetch & jquery (#6138)

* Added top-level exports
* Updated generator README
* Updated typescript generator docs

* Allow browsers File type for files (#5521)

* Allow passing file parameters as File objects
* Add test for jquery upload
* Use HttpFile object for node platform
* Regenerate samples

This is by far the most common use case. A `File` object already
contains the name attribute. This commit allows that information to be
used directly.
When sending a `Blob`, in most browsers the `File` constructor can be
used to assign a file name. In all other browsers the alternative is
```typescript
Object.assign(data, { name: "foobar.txt" });
```
That is why we explicitely pass the name as third parameter to
`FormData.append`. This `Object.assign` method also works for `Buffer`
objects in node.

If one really does not want to touch the data object in the browser it
is possible to define another reference to the data with
```typescript
new Blob([data], { type: data.type })
```
or in node via
```typescript
Buffer.from(data)
```

* [TS-Refactor] Added options for npm version, repository, name and updated readme (#6139)

* Added options for npm version, repository, name and updated readme

* Removed `this`  where not required

* Updated typescript docs

* Typescript refactor fixes (#6027)

Fixes a handful of issues identified in #802 (comment)

List of changes

* Clean: Remove redundant cliOption definition

* Remove redundant directory structure in templates

If we need to have different index.ts files for the different
frameworks, we can mostly do that in the one mustache file. In the cases
where that is not possible, we can still add a new override file later.

* Use File.separator consistently

* Only export selected api type

* Simplify promise polyfill import

The behaviour should be the same, according to the es6-promise docs.
Previously tsc would report the error:
> error TS2307: Cannot find module 'es6-promise'.

* Import HttpFile in all models

* Export server configurations

* Use undefined as default body value

The empty string is not interpreted as "no body" by the browser fetch
api and thus leads to an exception during get requests

* Improve codestyle: prefer guards to nesting

* Remove verbose debug output

This should not be commited, because every developer has very different
requirements what debug information he needs to see.

* Fix: Use cleaned model names for imports

* Fix: do not call toString on undefined

* Fix typo in doc comment

* Introduce RequestBody type and remove method check

* Support media types other than json (#6177)

List of changes:

* Add */* as fallback to accept header

* Use more sophisticated media type selection

* Handle object stringify in ObjectSerializer

* Parse response with ObejctSerializer

* Fix: Correctly extract response headers in browser

* Create HttpFile objects from responses

* Handle binary responses

* Clean up dependencies and replace isomorphic-fetch

Instead of isomorphic-fetch, which is unmaintained, we directly use
node-fetch and whatwg-fetch polyfills.

* Updated versions in ts-default/jquery and ts docs

* Replaced isSuccessCode with is2xx

* [TypeScript-Refactor] Use OAIv3 spec and fix bugs in JQuery Blob download (#6416)

* Change to OAIv3 spec for TS-Refactor

* Moved samples to oaiv3 folder

* Updated package-lock

* Update pom to use OAIv3 paths for Typescript-refactor

* Renamed ts-refactor samples & tests in pom.xmls

* Fixed compile issues in ts-refactor jquery http test

* Fixed bugs in blob handling of jquery

* [Typescript] Support http bearer authentication with token provider (#6425)

* Add http bearer security

* Update typescript to 3.9

* Fix: Use Authorization header for basic and bearer

* Allow asynchronous tokenProvider in bearer auth

* Add TS-Rewrite-Jquery tests node_modules to travis caching

* Remove NoAuthentication

* Added file to generate TS samples on Windows

* Exclude btoa in browser

* Regen samples

* Remove outdated ToDo comments

* Document and optimize `getReturnType` in TSClientCodegen

* Added option to generate objects for operation function arguments

* Upgrade typescript docs

* Updated generators

* Updated samples

* Updated docs

* Readded pom.xml

* [Typescript] Support InversifyJS (#6489)

* Add config option to enable InversifyJS

* Add pascal case lambda for mustache

* Generate a class for each auth method

* Add service identifiers and service binder helper

* Split Configuration into interface and factory

This way we don't need to import the factory everywhere to do
typechecking.

* Define minimal interface for ServerConfiguration

* Add annotations for inversify when enabled

* Always expose list of server configurations

* Add samples and defalt tests for useInversify

* Simplify sample generation script

* Fix: Add object_params arg description to help

* Fix: Properly enable inversify with bool property

* Build tests in pom instead of prepublish

Otherwise running `npm install`, when the build failed was impossible.

* Update dependencies for inversify tests

* Test basic api service resolution

* Remove Promise and Observable prefix from exports

* Fix, RxJS: Import Observable in object params api

* Add ioc service identifier for object param api

* Add hint about unimpeded development

* Simplify api service binder syntax

* Remove default tests for inversify

* Add wrapper for easy promise based http libraries

This wrapper allows defining and injecting http libraries that do not
need to know anything about observables, especially when useRxJS is not
enabled. I will employ this in the tests for InversifyJS.

Not sure if we should also use this wrapper internally.

* Add named injects for remaining auth parameters

* Directly inject promise services without RxJS

* Add tests for api service binder

* Add convenience method to bind all api services

* Fix: Rename inversify test artifact

* Run bin/utils/copy-to-website.sh

* Restore changes to CONTRIBUTING.md from PR #6489

Co-authored-by: Bodo Graumann <[email protected]>
Co-authored-by: Esteban Gehring <[email protected]>
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