Improving the network shim

[Mohammer5]

I was just reading through the network shim's code to get a better understading of its inner workings so I can review dhis2/cli-utils-cypress#176 properly.

After having read through the code, I like the way it's designed. It's quite straight forward and wouldn't change that at all.
I'm wondering about a few things though:

1. Open source, dhis2-agnostic network shim library

To me it'd make a lot of sense to create a standalone repository for the network shim, as it's quite dhis2 agnostic (except for the list of default static resources, which could be extracted). That way we expose the functionality to others out there as well (true open source, yay). I think it could be quite a useful resource to other cypress users out there, but if it remains in our cypress package, no one will find and/or use it.

2. Provide types or classes that are used in both the commands as well as the plugin code through a separate package to both packages

The types are defined in the createState.js. This makes it quite hard to have access to these, especially in the commands package. I have to fall back to the createState file of the plugins package while reading the code to know what is what.
I'm wondering how we could improve that without too much effort. One way we could improve it is with simple classes instead of type definitions (NetworkShimState, NetworkShimConfig, RequestStub), then reading the code inside the commands package is a bit more clear, e. g:

// from
state.requestStubs.push({                                                                                                                              
  path,                                                                                                                                                
  testName: isStatic ? null : testName,                                                                                                                
  static: isStatic,                                                                                                                                    
  count: 1,                                                                                                                                            
  nonDeterministic: false,                                                                                                                             
  method: request.method,                                                                                                                              
  requestBody: request.body,                                                                                                                           
  requestHeaders: request.headers,                                                                                                                     
  statusCode: response.statusCode,                                                                                                                     
  responseBody: scrubbedText,                                                                                                                          
  responseSize: size,                                                                                                                                  
  responseHeaders: clearDateFromResponseHeaders(response.headers),                                                                                     
})

// to
state.requestStubs.push(
  new RequestStub({
    path,                                                                                                                                                
    testName: isStatic ? null : testName,                                                                                                                
    static: isStatic,                                                                                                                                    
    count: 1,                                                                                                                                            
    nonDeterministic: false,                                                                                                                             
    method: request.method,                                                                                                                              
    requestBody: request.body,                                                                                                                           
    requestHeaders: request.headers,                                                                                                                     
    statusCode: response.statusCode,                                                                                                                     
    responseBody: scrubbedText,                                                                                                                          
    responseSize: size,                                                                                                                                  
    responseHeaders: clearDateFromResponseHeaders(response.headers),                                                                                     
  })
)

This would

• improve readability
• enable us to add additional checks to make sure that a new instance is valid
• give us the option to add immutability to these (which would simplify maintaining/extending/debugging the network shim)

3. Use debug module

If we used the debug module, we could log the various steps while using the network shim inside an app, would be quite nice to see if everything is working as expected or not

---

cc @HendrikThePendric

[HendrikThePendric]

Great ideas, I will provide some feedback on each one:

1. Open source, dhis2-agnostic network shim library

I'd be veery much in favour of this, but should point out that there are some other DHIS2 specific aspects to the current implementation apart from the default static resources:

1. Quite a lot of the variable names are DHIS2 specific, i.e. dhis2_base_url, dhis2_server_minor_version. This can obviously be changed into more general names fairly easily.
2. The NetworkShim was built with our feature-toggling-apps in mind. So the underlying assumption is that there are various server versions you would want to check against. I can imagine this would also be a useful generic feature, but as a more generic tool we should probably also support testing a single api version. So some changes are probably going to be required there.
3. Related to point 2, we currently verify that the server version specified in the NetworkShim does in fact match the reported server version. This code is highly specific to DHIS2, we probably need to expose this as some form of hook. Or perhaps from the generic network-shim package we just support a bunch of functions, and let people create their own enableNetworkShim. For example we can expose these
   • The networkShim plugin
   • The following helpers for setting up the capturing/stubbing logic in ./cypress/support/index.js: isDisabledMode, isStubMode, isCaptureMode, captureRequests and stubRequests.

On balance, I still think this is a good idea, but we need to consider the additional work and discuss it with @varl. Apart from the time it will take to extract a generic implementation from the current implementation and let the dhis2/cli-utils-cypress use that, we also need to consider the long term investment in terms of support and adding features we might not directly need.

---

2. Provide types or classes that are used in both the commands as well as the plugin code through a separate package to both packages

I agree the way the types have been declared in the code is pretty quirky and should be improved. My personal preference would be to not use classes for this but to add TypeScript Type Declaration files, see here. I don't have experience with this type of thing, but it seems to be a good solution here.

One thing we should consider as well, is our generated documentation. It's probably possible to generate docs from these *.d.ts files, but I'm not sure this is supported by our tooling at this moment in time. I added the jsDocs comments so that we would be able to auto-generate the API docs (even though I've just realised this isn't being done 🤷‍♂️ ). I don't want to end up in a situation where we have to maintain actual types and the jsDocs simultaneously.

---

3. Use debug module

Yeah, this would be good. Additionally, we need a "debug-mode" in the package, probably this should be some command in the example-app. It needs to do the following:

• Start a cypress run
• Use headed chrome
• Pass the --no-exit flag to cypress run

The reason we need this, is because:

• cypress run and cypress open behave slightly differently and the NetworkShim should function properly when doing a run (because we intent to run it mainly on CI).
• When in run mode with a headless browser there is no way to inspect the browser console, hence the need for using a headed browser
• Without passing the --no-exit flag the browser will close after the test, so it's still not possible to view the browser console

The above is needed, and when using the d2-utils-cypress run command it is not really possible ATM (I believe we aren't forwarding flags properly 🤷‍♂️ ). I guess if we extract the NetworkShim and its example-app into a separate repo, we'll not be using d2-utils-cypress but issuing cypress commands directly, so we can just fix it there. Although I believe that forwarding flags to cypress run and cypress open should be supported via the equivalent d2-utils-cypress commands.

[HendrikThePendric]

Additionally, I just wanted to point out that even if we were to go ahead with all of the above, these points shouldn't need to cause breaking changes in dhis2/cypress-commands or dhis2/cypress-plugins. As such, I think we should proceed to merge in our current PR dhis2/cli-utils-cypress#176 , which is going to trigger breaking changes, but does fix an important issue.