kie-issues#1694: Apache KIE Extended Services extension issues

[yesamer]

Fixes apache/incubator-kie-issues#1684

In this PR:

The main issue is the "ambitious" management of the Extended Service Quarkus App lifecycle.
The original idea was to run the Quarkus application just after a DMN / BPMN file is opened in the editor and close it when all files are closed. Unfortunately, that implementation didn't consider the following scenarios:

• Validation checks are called when the Quarkus Application is starting up and not still ready
• Heartbeat requests are called in the same scenario described previously
• Close and opening a single DMN/BPMN file at the same moment leads the App to reach an inconsistent state, as the request of closing and starting are launched at the same moment

For that reason, I simplified the logic: the Extended Service Quarkus App is started when a DMN / BPMN file is opened for the first time and never stopped. The original idea was indeed better from a performance and resource consumption point of view, but it requires a more complex implementation.

• Updated node-fetch to 3.3.2
• extendedServices.connectionHeartbeatIntervalinSecs default value changed from 1 to 10. Having a heartbeat occurring every second contributed to the reported issue
• Introduced Debug logging
• Improved error message management
• Improved Configuration management
• Improved validation call body management, using the same terminology used in JIT (backend)

[ljmotta]

@yesamer Thanks for your PR! I've made some comments inline.

[ljmotta]

We should use the env value instead of hardcoding. To do so, take a look into the build/defaultEnvJson.ts file in some packages (e.g. online-editor).

[yesamer]

@ljmotta It's a good point. I would like to retrieve that default value directly from the configuration here https://github.com/apache/incubator-kie-tools/pull/2802/files#diff-153d43651b25ee3120020fae349c9e8ed99ccf90aa15995784702244decf9499R91 to avoid duplication and a single place for that property value, do you think is a valid alternative should I consider?

[jomarko]

yes, it is important to have an option to configure the url of extended services, users report issues about it #2759

[yesamer]

@ljmotta In the examples I checked, the env variables are accessed using the useEnv() hook, but in this case, we are not managing a React app. How can I access to env variable from "plain" typescript?

[ljmotta]

@yesamer That's true. the defaultEnvJson is used with the webpack transform + fetch, which isn't the case here. Now, you could use the webpack's EnvironmentPlugin to set env variables. The online-editor uses this approach to set some values inside the codebase. Take a look into the online-editor/webpack.config.ts, and search for EnvironmentPlugin. The WEBPACK_REPLACE__* values are the ones used in the codebase.

[yesamer]

@ljmotta It should be done, there are two points to discuss:

• I added EnvironmentPlugin in both extension-main and extension-browser. There's a way to define that once?
• Currently, the host and the post are parametrized, In my opinion, it would be better to have the entire URL as a parameter. E.g. http protocol is hardcoded, what if the user provides its own instance using https?

[ljmotta]

@yesamer

• If you need to use in both, you can set the env variables in the commonConfig variable.
• Good point. I'm not familiar on how the user set/use the Extended Services in the VS Code extension. Is it a embeded Extended Services or the user need to manually start the server? For the extended-services-java it will always use http. If the user wants to deploy it, the user will need to make the necessary adjustments to make it available through https.

[yesamer]

@ljmotta

• Do you have an example for that? I mean, I added the plugin entry for both web and node https://github.com/apache/incubator-kie-tools/pull/2802/files#diff-1768335dedd5cfa5e1b3830bb3b6c504f1d316dea21de55738df74f7f19c27a5R52
  To create a single entry for the plugin, which target and entry should I use?
• You're right, considering extended-services-java always uses http, we are safe here :)

[ljmotta]

I'm not sure if I followed. T can be a boolean, number or string. If is a boolean it will always be true, as it is the default value. For number, can't be 0, and string can't be ""? Or value can't be undefined? The (!value) can be tricky, please, make it explicity.

[yesamer]

T can be a boolean, number or string. If is a boolean it will always be true, as it is the default value. For number, can't be 0, and string can't be ""

So, if the user explicitly removes the property the default value is always returned?

[ljmotta]

@yesamer I just considered the calls to this method. We have just 3 on this file, each one of them with T equals to boolean, number or string. The if will be true for the cases I listed, and we will have the log. Additionally, the return will always go to the default value for the same cases:

> "" || "my_default_value"
'my_default_value'
> 0 || 25
25
> false || true
true

If the value (has the type T due your casting) can have a different type, please make it explicity. Something like: T | null or T | undefined.

[jomarko]

Thank you for the PR. I am happy we improve this extension.

01

Sometimes we use extended service (singletone) and sometimes we use extended services (plural).

02

The PR description says we never stop the extended services. Should then extension-browser.ts contain const stopExtendedServicesCommandUID: string = "extended-services-vscode-extension.stopExtendedServices"; and related code? Or do we keep the option to manually stop the extended services?

No manual check done, waiting until code changes are finished.

[jomarko]

yes, it is important to have an option to configure the url of extended services, users report issues about it #2759

[yesamer]

@jomarko
01
I updated all the references to use the plural form

02
Yes, the user can close the connection using a button in the bottom bar of the editor.