Repository navigation
Conversation
buildNodeUrl now hands each linked path to the Java engine, which keeps it for the resource being rendered. A render filter at priority 17 registers the paths before the cache filters store the fragment. The React renderer and JSScript return to the string contract they had before. - A call that passes its own context now collects, because the collection needs only the render context. - Above 100 paths, the merge groups the paths by site and never builds a regexp above /sites/<key>. A lone path of another site stays exact. - Paths under /modules are dropped, as the core cache filters drop them.
🦜 Chachalog
|
commit: |
The filter now pushes a collector for each resource in prepare, with RenderChain.pushAttribute. A nested render pushes its own collector, and the render chain restores the collector of the parent when that render ends. This replaces the request-wide map of the resources. The view property is read once, at the first collected path, because the script of the resource is then resolved. A view that opted out stores no path.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The reducer can exceed its documented dependency limit when paths span many distinct scopes.
2 open findings
What changed in this PR
Moves automatic URL cache-dependency collection into a render filter while restoring string-based renderer results.
Changes:
- Collects dependencies through the active render context.
- Reduces dependencies per site and drops module paths.
- Extends unit and end-to-end coverage.
| File | Description |
|---|---|
tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts |
Tests explicit contexts and per-call opt-out. |
javascript-modules-library/src/utils/urlBuilder/urlBuilder.ts |
Delegates dependency collection to the render helper. |
javascript-modules-library/src/hooks/useServerContext.tsx |
Removes React-specific collection state. |
javascript-modules-engine/src/server/init-react.tsx |
Restores string renderer results. |
javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java |
Expands collection and reduction tests. |
javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/views/JSScript.java |
Restores the renderer’s string contract. |
javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java |
Exposes render-context dependency collection. |
javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java |
Adds request-scoped collection and registration. |
javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java |
Implements scoped collection and grouped reduction. |
jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx |
Adds integration-test views. |
.chachalog/Kp7mQx2R.md |
Updates the release note. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…the limit needs The filter now has the conditions of AggregateCacheFilter: live mode, no json, no include, wrapper or option, no fragment cached with its parent, and no render under skip aggregation. A resource the cache does not store gets no collector, so its view collects into the fragment that holds its markup. The filter reads the opt-out of the view in prepare, because NodeAttributesFilter has already resolved the script at priority 16.8. Above 100 paths, the merge takes the deepest folder that covers two entries or more, and repeats until the entries fit. A lone link outside a listing stays exact. No merge goes above /sites/<key>, and a warning reports a result that stays above the limit.
…rent A view renders a virtual node with Render content, which the render helper marks to be cached with its parent fragment. The view of the virtual node builds the image URL, and the rename of the image must flush the parent. The Javadoc of the filter states that the opt-out of the enclosing view decides for a view that renders inside its fragment.
A test generates 500 sets of paths over sites, user folders and modules, with a fixed seed. On each set it checks that no path is lost, that nothing merges within the limit, that no merge goes above /sites/<key>, that each merged folder holds two paths or more, that no entry lies inside a merged folder, and that a result above the limit has no folder left to merge. The merge now groups the entries once per depth, from the deepest. Two folders at one depth never overlap, so the result is the same, and 5000 paths in 2500 folders reduce in a few milliseconds. Other tests cover the boundary at 100 paths, a deeper merge absorbed by a higher one, an exact path equal to the merged folder, the paths above the floor, and what register() puts on the resource.
buildNodeUrl reads its context through JavaScript calls only, so a view may pass an object that stands in for the render context. The test view of #811 does this to imitate a link to another site. autocollectCacheDependency took a RenderContext, and GraalVM cannot convert such an object to that Java type, so the render failed. The method now takes the request, which buildNodeUrl reads with getRequest().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request moves the collection of the
buildNodeUrlcache dependencies out of the React renderer. A render filter now registers the collected paths, and the merge above 100 paths no longer covers more than one site.Why
#783 kept the collected paths in a
Setof the React server context. The React renderer returned that set next to the markup, andJSScriptread the new{html, autocollectedDependencies}result. Three layers had to change together, and the design had three consequences:buildNodeUrlthat passes its owncontextcollected nothing, because that object carries no set.getImagePropsand any helper outside a component make this call.JSScript, becauseJSScriptread a member of an object./sites/x/filesmerged to/sites/x(/.*)?, and one link under/modulesmerged to/.*, which matches every path of the repository.Changes
buildNodeUrlcallsserver.render.autocollectCacheDependency(path, renderContext.getRequest()). The collection needs only the request, so a call that passes its own context collects too. An object that stands in for the render context, as in the test view of fix(library): keep the origin of a URL that encodeURL made absolute #811, works as well.CollectedCacheDependenciesFilterruns at priority 17. Inprepare, it pushes a collector for the resource withRenderChain.pushAttribute, and the render chain restores the collector of the parent when a nested render ends. Inexecute, the filter registers what the collector holds, before the cache filter at 16 stores the fragment.AggregateCacheFilter: live mode, nojson, noinclude,wrapperoroption, no fragment cached with its parent, and no render under skip aggregation. A resource the cache does not store gets no collector, so its view collects into the fragment that holds its markup.cache.autocollectDependenciesfrom the properties of the view inprepare.NodeAttributesFilterhas already resolved the script at priority 16.8. A view that opted out collects nothing./sites/<key>, and a warning reports a result that stays above the limit. The merge drops the paths under/modules, because the core cache filters store no dependency there.JSScriptreturn to the string contract.ServerContextloses the internalautocollectedDependenciesfield.Validation
mvn -pl javascript-modules-engine-java -am test -Dtest='CollectedCacheDependencies*Test': 24 tests pass. One test generates 500 sets of paths with a fixed seed, and checks the invariants of the merge on each: no path lost, nothing merged within the limit, no merge above/sites/<key>, no redundant entry, and no folder left to merge above the limit. The test fails on each of three bugs injected into the merge. The worst case, 5000 paths in 2500 folders, reduces in 6 ms.mvn -pl javascript-modules-engine-java,javascript-modules-library,javascript-modules-engine -am package -DskipTests: the generated types carryautocollectCacheDependency, and the library and the engine compile against them.cacheDependencyAutocollectTest.cy.tsgains four views that must be flushed and one that must stay cached. The flushed views pass their context explicitly, build the URL in an include of the current node, or build it in a virtual node thatRender contentrenders. The cached view opts out for a single call. The spec passes in the standalone integration tests, on thejahia-ee-dev:8-SNAPSHOTimage.Documentation
The changelog entry of #783 is not released yet. It now names this pull request and states how a view with more than 100 links refreshes.
ADR
None.