SAP / spartacus

Spartacus is a lean, Angular-based JavaScript storefront for SAP Commerce Cloud that communicates exclusively through the Commerce REST API.
Apache License 2.0
744 stars 389 forks source link

In SSR: translation chunk JSON files should be read directly from the filesystem #13460

Closed neilhubertprice closed 1 year ago

neilhubertprice commented 3 years ago

SSR currently follows the client behaviour for loading translation chunk JSON files: each is requested individual via an HTTP request. In CCv2 with the 2.x and 3.0 versions this goes via loopback on the localhost address, from 3.1 onwards a fix to the host name handling means it goes to the jsapp endpoint. Either way it still creates multiple HTTP requests for a single SSR page request (2 per chunk, for main + fallback language). This amplifies the number of requests coming into the node.js server or jsapps endpoint (depending on the version) & that potentially has implications for denial of service style attacks.

Spartacus versions: 2.x and above Reproduction: Setup Spartacus SSR with translation chunks, request SSR page

The underlying package that is used here is i18next, which has multiple different backends that it can use for loading the files, see https://www.i18next.com/overview/plugins-and-utils#backends The Spartacus code is currently using i18next-xhr-backend (replaced with i18next-http-backend in Spartacus 4.0 I think). However there are also backends for direct filesystem access in node.js, see https://github.com/i18next/i18next-fs-backend & also there is a capability to chain different backends, see https://github.com/i18next/i18next-chained-backend.

Spartacus should include a capability to switch the i18n backend to filesystem when in SSR mode - perhaps with a chained backend so that it can fallback to HTTP requests if necessary. This is both a performance (remove unnecessary HTTP request) and security (potential for DoS) fix in my view.

Xymmer commented 3 years ago

We'll aim to do this for next major

artem-zur commented 2 years ago

Hey there!

Any plans to start supporting 'i18next-chained-backend'? In our project we start using Locize as a solution for managing translations. We wanted to implement a backend fallback but it seems impossible to do this right now without deep customization.

P.S. Would be very appreciative if somebody can help with a workaround.

Platonn commented 1 year ago

In case when ExpressJS it’s overloaded with requests (which can happen when we have too big currency option value in OptimizedSsrEngine options), then the queue of requests handled concurrently in ExpressJS’s can become big. Then the NodeJS is trying to render too many requests concurrently, making the requests to be responded slow.

This situation is bad by itself. And in this situation I’d recommend having low concurrency (ideally 10, eventually up to 20) .

The http requests for JSON translations in SSR are currently obtained via http calls, so it goes through the ExpressJS server. So in case of overloaded ExpressJS server, waiting for those JSONs in the rendered applications takes long time. Therefore the rendering of those applications gets even longer.

Platonn commented 1 year ago

When using lazy-loaded translations, in SSR we prepend the full origin to the relative path, so the url becomes absolute. Therefore every request for a translation resource must leave the SSR pod, then go through other parts of an internal infra, then the request is routed back to the SSR Express.js server, which eventually serves the file content from the folder /assets.

This sounds like putting an unnecessarily a load on ExpressJS server and on an internal network infrastructure. Instead it would be far more reasonable to load JSON translations in SSR directly from the filesystem.

Platonn commented 1 year ago

Tracked in https://jira.tools.sap/browse/CXSPA-2060

Platonn commented 1 year ago

Fixed in https://github.com/SAP/spartacus/pull/16791 . Release planned for 6.1