fiori-tools-proxy disables npm module shims in its' default config

vobu commented 2 years ago

Description

The issue was initially reported at https://answers.sap.com/questions/13461279/ui5-project-shims-not-working.html?childToView=13479438.

When a service is added using the odata-service template. the fiori-tools-proxy middleware is added to the ui5.yaml

server:
  customMiddleware:
  - name: fiori-tools-proxy
    afterMiddleware: compression
    configuration:
# ...
      ui5:
        path:
        - /resources # <-- this line makes the proxy intercept everything with the prefix /resources
        - /test-resources
        url: https://ui5.sap.com
        version:  # The UI5 version, for instance, 1.78.1. Empty means latest version

If a developer then wants to using the shimming functionality and adds a configuration like

specVersion: '2.1'
kind: extension
type: project-shim
metadata:
  name: thirdparty # this can be your project shim name
shims:
  configurations:
    lodash: # name as defined in package.json
      specVersion: '2.1'
      type: module # Use module type
      metadata:
        name: lodash
      resources:
        configuration:
          paths:
            /resources/thirdparty/lodash/: ''  # never getting here because the proxy intercepts it before

It doesn't work because the shimming capability requires resources not to be generically proxied, as it relies on a subdir of resources to make the npm module available at runtime.

Steps to Reproduce

Steps to reproduce the behavior:

Run the tests in the fiori-freestyle template
Enhance the one of the generated projects in the test-output folders with the shim configuration above
Try reaching any file in /resources/thirdparty/lodash
See 404 not found error

Expected results

Actual file is returned.

Actual results

404

Screenshots

-n/a-

Version/Components/Environment

Add any other context about the problem here OS:

[x] Mac OS
[x] Windows
[x] Other

Root Cause Analysis

Problem

See description

Fix

Different options

Change the generated ui5.yaml to only proxy requests start with sap
```
ui5:
path:
- /resources/sap*
```
Leave the template as-is and report the issue to the fiori-tools-proxy team
Leave it as is and document that users that require shimming have to change the default settings

Why was it missed

This feature is not covered in a test case.

How can we avoid this

Create a test case covering it.

pinging @petermuessig @tobiasqueck as FYI

tobiasqueck commented 2 years ago

@Klaus-Keller what do you think? Is this something that should be fixed in the fiori-tools-proxy or should we update the templates as @vobu suggested as fix option 1?

@petermuessig is there anything not starting with sap at https://ui5.sap.com that we could/would/might need during the preview?

Klaus-Keller commented 2 years ago

@petermuessig, for custom middleware we have a defined execution order and can also influence this execution order, for instance using

afterMiddleware: compression

Does such an option also exist for project shims? Couldn't find a way to influence execution order in https://sap.github.io/ui5-tooling/pages/extensibility/ProjectShims/

tobiasqueck commented 2 years ago

@RandomByte could you help us here?

RandomByte commented 2 years ago

It doesn't really matter whether it's a shim or an already configured dependency of type module. From what I understand from this issue, the standard middleware serveResources is never reached for resources that have a path starting with /resources. See here for the order of standard middleware in ui5-server.

Without knowing the implementation, I understand that the fiori-tools-proxy is "eager" and tries to proxy everything that does not belong to the application. Would it be possible, in case of a 404 from the proxy target, to call the next middleware (next()) in the proxy? I think that should already solve this issue.

zdravko-georgiev commented 2 years ago

Hi @RandomByte, it is a good idea, but unfortunately it doesn't work as expected. It is due to the fact that the http-proxy-middleware (the middleware that we are using in the fiori-tools-proxy) doesn't really support this feature (please see Issue248 for reference).

I can call next(), but then I get an exception Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client, because res.end() was called as soon as 404 was received.

petermuessig commented 2 years ago

Hi all, and sorry for joining the party that late. Basically, the UI5 resources of today are typically in the resources/sap* location but there are some special cases for the jQuery plugins, the ui5loader.js or ui5loader-autoconfig.js.

I like the suggestion of Merlin but this would mean the proxy middleware would require a response stub which is used to intercept the calls like setting the headers and res.end() - I did this in Java for some Servlets in the past and this may also work for NodeJS. Maybe just put a Proxy around and intercept the required functions like setHeader or end.

Cheers, Peter

RandomByte commented 2 years ago

@zdravko-georgiev Yes I remember running into the same issue with my proxy PoCs. I guess we only have two options then:

Defining a more restrictive pattern to match for the proxy middleware
Moving the proxy middleware after the serveResources standard middleware. Ideally next to the connectUi5Proxy: https://sap.github.io/ui5-tooling/pages/Server/#standard-middleware
- This has the drawback that if there are framework dependencies defined in ui5.yaml, these will be served from the local system instead of the proxy target. This can lead to serious issues if the UI5 version does not match.

Maybe we need to talk about better integration for proxies into the ui5-server after all...

zdravko-georgiev commented 2 years ago

@RandomByte @Klaus-Keller I have another idea. Maybe we can define an additional property e.g. ignoreList where the user can explicitly define list of paths that should be ignored by the proxy. Theoretically this is already possible since one can enter also a Regex as a path https://expressjs.com/en/4x/api.html#path-examples. But we can make it more user friendly, e.g.

ui5:
  path: 
  - /resources
  - /test-resources
  ignoreList:
  - /resources/thirdparty/lodash
  url: https://ui5.sap.com
  version:  # The UI5 version, for instance, 1.78.1. Empty means latest version

RandomByte commented 2 years ago

Sure, that would work. But I don't find it very user friendly. The UI5 Tooling already knows that there is a non-framework dependency with path /resources/thirdparty/lodash. Ideally, there shouldn't be a need to have the user provide this path to the proxy middleware.

With your solution the user would need to know the paths a module dependency (and this might even be a transitive dependency) has configured in its ui5.yaml. These paths could even change in newer versions of the dependency, forcing the user to adapt their proxy config when updating dependencies. Also, how does the user learn that when adding a module dependency, the proxy configuration needs to be adapted as well?

zdravko-georgiev commented 2 years ago

I see your point.

The UI5 Tooling already knows that there is a non-framework dependency with path /resources/thirdparty/lodash.

Just out of curiosity. How does the UI5 Tooling knows?

Also, how does the user learn that when adding a module dependency, the proxy configuration needs to be adapted as well?

Documentation is key

RandomByte commented 2 years ago

The UI5 Tooling already knows that there is a non-framework dependency with path /resources/thirdparty/lodash.

Just out of curiosity. How does the UI5 Tooling knows?

Because it's the one providing these dependencies. ui5-server could certainly implement a functionality to skip a proxy middleware for resources of non-framework dependencies if this is wished by the user.

But this would be beyond the scope of the current custom middleware extensibility and require a concept for some kind of "proxy extensibility" with a central configuration in the ui5.yaml

vobu commented 2 years ago

Also, how does the user learn that when adding a module dependency, the proxy configuration needs to be adapted as well?

Documentation is key

not trolling here, but FYI: there is no documentation to the outside (developer) world. yet. but hopefully will be, once you people Open-Source this proxy.

vobu commented 2 years ago

Maybe we need to talk about better integration for proxies into the ui5-server after all...

🎆 🥳 🍾

zdravko-georgiev commented 2 years ago

Hi @vobu, all of the fiori-tools-proxy capabilities are documented in https://www.npmjs.com/package/@sap/ux-ui5-tooling#2-proxy.

Please let me know if you are missing something and I will update it.

vobu commented 2 years ago

Hi @vobu, all of the fiori-tools-proxy capabilities are documented in https://www.npmjs.com/package/@sap/ux-ui5-tooling#2-proxy.

ah, thx for pointing me there, didn't know that one yet

zdravko-georgiev commented 2 years ago

Update

We decided to go for the idea where the fiori-tools-proxy calls the next available middleware (next()), if the proxy target returns 404. The fix is currently being implemented. Afterwards it will undergo quality assurance. If it all goes well we should have it released in 3-4 weeks.

zdravko-georgiev commented 2 years ago

Update

The fix didn't make it in the next release. Now it should be released mid of January.

zdravko-georgiev commented 2 years ago

Update

Fix was submitted to the main branch. The fix will now undergo quality assurance and if all goes well it will be released at the end of this month.

zdravko-georgiev commented 2 years ago

Fixed with https://www.npmjs.com/package/@sap/ux-ui5-tooling/v/1.4.7.

zdravko-georgiev commented 2 years ago

Hello, we needed to revert the fix, because the fix was not stable. There were multiple reports of the error Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client. At this point in time we will not provide a fix for this. Please refer to https://answers.sap.com/questions/13461279/ui5-project-shims-not-working.html?childToView=13479438 for the workaround.

Best regards, Zdravko

vobu commented 2 years ago

well, the workaround in https://answers.sap.com/questions/13461279/ui5-project-shims-not-working.html?childToView=13479438 is not really a workaround as a pattern like

ui5:
        path: 
        - /resources/sap*

will not proxy ui5-loader* or any jquery* resources.

tobiasqueck commented 2 years ago

The sources responsible have been published by now and are at https://github.com/SAP/open-ux-tools/tree/main/packages/ui5-proxy-middleware. @vobu any proposal how to fix it? Should we reopen the issue?

Btw, wouldn't the config below solve the issue?

ui5:
        path: 
        - /resources/sap*
        - /resources/ui5-loader*
        - /resources/jquery*

proehlen commented 6 months ago

Hello, we needed to revert the fix, because the fix was not stable.

Hi just enquiring if a fix is still being considered for this or if the issue has been moved to a new discussion?

Edit: in the meantime I can confirm that @tobiasqueck 's workaround appears to work for me.

2nd Edit: actually, the work around doesn't seem to be reliable:

ui5loader-dbg.js:1166 Uncaught (in promise) ModuleError: Failed to resolve dependencies of 'sap/ushell/Container.js'
 -> 'sap/ushell/renderer/ShellLayout.js': failed to load 'sap/ushell/renderer/ShellLayout.js' from ../resources/sap/ushell/renderer/ShellLayout.js: script load error

zdravko-georgiev commented 6 months ago

Hi @proehlen, currently we are not considering to provide a fix for this issue.

SAP / open-ux-tools