TS-78 NetworkHttp now uses NetworkRoutes - Generated open api lib

[fboucquez]

Removed open api generated code. Added new generated lib dependency.
Updated typescript compiler to fix a lib problem after a "npm install"

This PR uses the brand new open api client generator
https://github.com/NEMStudios/nem2-open-api-generator

and the generated library
https://www.npmjs.com/package/nem2-sdk-openapi-typescript-node-client

[coveralls]

Pull Request Test Coverage Report for Build 817

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 815:	0.0%
Covered Lines:	10
Relevant Lines:	10

---

💛 - Coveralls

[evias]

#342

I would want to discuss that one as it is:

• 1. adding complexity that we removed 3 months ago (nem2-library).
• 2. removes compatibility for browser builds (because yet another library must be made browser ready)
• 3. is not addressing what it should address (“change network routes to use network type”)

—

1. The sub-library principle has been intensively discussed over the last two years, in fact nem2-library was our “generated code lib” but this is only making things much more complicated to maintain and doesn’t make sense if it is client dependent like the one introduced “node-client” openapi package. That one would be one more package that needs a browser build. (and makes it complicated to track and maintain nem2-sdk builds)

2. Browser/clients builds of the sdk are using polyfills for react, vuejs, etc. for the libraries that are node-dependent. This is an important knowledge that cannot be changed for now or that would make all clients incompatible again. With the newly introduced node-dependent openapi package, this will be more one more package for which to track more than one build.

3. Please rename the PR to what it actually does: Change the SDK to use a sub-library for handling all Api generated classes.

[evias]

On another note, it was pointed out by the catapult-rest team that the /network route should not be used to determine the type of network. This is because the name field of the response of /network could be anything ; it doesn't have a rule set about returning only mijin, mijinTest, public or publicTest; it can be anything.

While /node/info in fact can only return one of the four values.

Note that /node/info may need to be repositioned in the library as its not a Network route (could be in NodeHttp).

[evias]

@dgarcia360

[fboucquez]

@evias

Thanks for the comment.

i) What was the library removed 3 months ago? This library is just an NPM package of the generated ts/js files from using swagger/open API cli.

ii) How are these generated files made "browser ready"? are they manually modified?

iii) Open API spec was changed for this, now the possible values are enum values, no free text. In order to complete the task, I needed to regenerate the code. I am not happy with pushing the generated code. So I included the technical improvement in this task. Many more open API changes are in the pipeline, having the nem2-open-api-generator will give us generated clients for free when we are working for example in another SDK like the java. The nem2-open-api-generator is generating libs for f2 based on the now split open API spec. This will be true if we need to generate clients for other languages like c#, python, etc. I have changed the PR title.

1. If the generated code is committed into the main project it will make it more client dependent. Having a separate module means that you can change that module. In Java, we have 2 generated clients, okhttp (android focus) and vertx (server focus). The generator use is "typescript node", the same used in the committed code. The generated code was done by a dev in a machine. The process was not documented. Now the script and tools to generate and publish the code are in Travis. You know what is used to generate the clients and update if necessary.

2. Are you forking the SDK repo to have different types of builds? nem2-open-api-generator can easily generate different types of clients (https://openapi-generator.tech/docs/generators.html).

Note from rest:

If the text is actually free text, the open API needs to be changed back to string and we would need to use another way of retrieving the network type. I would like to keep a one to one mapping between the repositories and routes. Network Repository should talk to the Network Route, Node Repository should be the one talking to the Node Route. NodeRepository should we the one returning the network type, Network Repository should return the name if it's free text. (This applies to Java too)

[dgarcia360]

@evias #287 (comment)

[evias]

Interesting @dgarcia360 thanks for pointing out ;

@rg911 did the Network sdk repository implement this change :

As a potential solution, NetworkHttp.getNetworkType() could parse the version property from the first block header.

That would keep the current structure and make /network good to retrieve the network identifier (which is still not the name produced by rest because rest reads that from configuration file, not from network.)

[evias]

@fboucquez on point 3) it is not correct:

• https://github.com/nemtech/catapult-rest/blob/master/rest/src/routes/networkRoutes.js#L25
• https://github.com/nemtech/catapult-rest/blob/master/rest/resources/rest.json#L2

This is where /network reads the nameand there is no formatting about it or check that it indeed holds one of the 4 values.

[evias]

@fboucquez adding to point 2) Currently our client applications use the single NPM library that is available but for react-native for example, a polyfill is provided for http replacements like :

http: require.resolve('stream-http'),
https: require.resolve('https-browserify'),

Now with what was introduced in nem2-qr-library, the npm run build.browser that would be taken care of by Webpack directly, have a look here:

• https://github.com/nemfoundation/nem2-qr-library/blob/master/package.json#L50
• https://github.com/nemfoundation/nem2-qr-library/blob/master/webpack.config.js

One note about this is that you have to name the sdk for the browser build ..

Discussions can be held about this but the fact that client integrations don't each need to polyfill would be helpful, so the proposition would be to implement the same for the SDK, but extending the following line: https://github.com/nemfoundation/nem2-qr-library/blob/master/webpack.config.js#L46 to use pre-defined polyfills.

That can always be changed by integrators of the SDK, by changing the webpack.config.js file for their specific build needs.

My concern with re-introducing multiple packages from which to track several parts of the SDK is that the nem2-library package did end up dead and introduced a lot of complexity to maintain the SDK.

--

Introducing a nem2-openapi-node-client will mean that client integrations need 2 packages built for browser, and they need the correct mix for the build to be working.

This is definitely an issue with SDKs that has been attacked on Java but I am unsure whether it makes sense to define multiple packages for different http implementations.

Ultimately, the http implementation should be pluggable and should not require any kind of dependency changes.

[fboucquez]

I've rolled back the network/node endpoint change. NetworkHttp uses the node route to get the network type. I've added NetworkInfo endpoint so the user can read the name and description from rest.

[fboucquez]

Hi @evias, was your team able to try the new library?

@dgarcia360 and I rolled back the enum to free text and we are using the node routes to get the network type. This pr should we merged for f2. The open api code should be handled by adding the library. Npm package for f2 is https://www.npmjs.com/package/nem2-sdk-openapi-typescript-node-client

[fboucquez]

This PR is stale now. It has been superseded by #409

@evias, could you have a look at this? I'll close this PR.