-
Notifications
You must be signed in to change notification settings - Fork 392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: CXSPA-6890 Create toggle and optimization options for propagating errors to the server #19021
Conversation
core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts
Outdated
Show resolved
Hide resolved
core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts
Outdated
Show resolved
Hide resolved
projects/core/src/features-config/feature-toggles/config/feature-toggles.ts
Outdated
Show resolved
Hide resolved
* Caching strategy resolver that determines the behavior of caching in RenderingCache. | ||
* By default, the caching strategy is based on the presence of an error. | ||
*/ | ||
cacheStrategyResolver?: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cacheStrategyResolver: (config, entry) => boolean
Maybe it's only me that see this name as very ambigious. On the first sight I don't know what is a "cacheStrategy", that it wants to resolve.
What about alternatives? Examples:
shouldCachePage: (config, entry) => boolean
cacheCondition: (config, entry) => boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind changing this property name. What do you think about:
shouldCacheResponse: (config, entry) => boolean
shouldCacheRendering: (config, entry) => boolean
shouldCacheRenderingResult: (config, entry) => boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldCacheRenderingResult
is most clear to me.
+to be future proof (for adding new properties/arguments), I'd wrap args in just a single object ({ config, entry }) => ...
PR contains toggle for propagating async errors captured during SSR to the server. Apart from that, it contains additional optimization options to configure the rendering cache - required as we're going to handle errors, which we discourage caching.
QA steps for
propagateErrorsToServer
toggle:pages
property indefault-cms-config.ts
to generate 'CmsPageNotFound` error.run dev:ssr
index.html
returned with status404
spartacus-features.module.ts
and insideprovideFeatureTogglesFactory
setpropagateErrorsToServer
to false200
(as it used to be before changes around ssr error handling)npx ts-node ./tools/schematics/testing.ts in the SPA root folder
npx @angular/cli@17 new my-app --standalone=false --ssr=false --style=scss --routing=false
npx @angular/cli@17 add @spartacus/schematics@latest --ssr
provideFeatureToggles({propagateErrorsToServer: true}) in
app.module.ts`server.ts
insideapp
function use handlers provided by Spartacus:npm run build
andnpm run serve:ssr:my-app
to verify whether the app fell back to CSR andindex.html
returned with status404
npm run build
andnpm run serve:ssr:my-app
and verify whether the rendered with status200
(as it used to be before changes around ssr error handling)QA steps for new optimization options
avoidCachingErrors
andcacheStrategyResolver
:server.ts
, passcache: true
to thessrOptions
components
property indefault-cms-config.ts
to generate async error in SSR.npm run dev:ssr
and run app in the browserindex.html
returned with status500
:server.ts
, addavoidCachingErrors: true
to thessrOptions
Render from cache (/electronics-spa/en/USD/)
and rendering was triggered each time the page was reloadedserver.ts
, add any customcacheStrategyResolver
tossrOptions
, e.g:cacheStrategyResolver
was executed (If the above example was used, check server logs)