Round-trip optimization for HTML resource views (#346) - #347
Merged
Conversation
Inline a small reader script in the mashlib HTML wrapper that exposes window.__dataIsland.get(uri) as a generic accessor and patches $rdf.fetcher.load() (when rdflib is present) to resolve from the inline JSON-LD data island instead of issuing a second HTTP request. Bounded retry (~10s) prevents infinite polling on non-rdflib clients. Falls through to original network fetch on any miss or parse error. Default-on; opt-out via opts.roundTripOptimization=false. Closes #346.
There was a problem hiding this comment.
Pull request overview
Adds an inline “data island reader” to JSS-generated Mashlib HTML wrappers so rdflib-based clients can satisfy fetcher.load(uri) directly from the embedded JSON-LD data island, avoiding a second HTTP fetch and reducing round-trips (per #346).
Changes:
- Introduces
roundTripOptimizationScript()and anopts.roundTripOptimization(defaulttrue) flag on Mashlib HTML generators. - Injects the reader script into CDN/local/module HTML wrappers immediately after the data island.
- Adds a new unit test suite to assert reader emission/opt-out and basic invariants (retry bound, no
</script>token, etc.).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/mashlib/index.js |
Adds the inline reader script and a default-on opt-out flag, and injects it into generated HTML wrappers. |
test/round-trip-optimization.test.js |
Adds tests validating the reader’s presence/absence and some safety/structure properties in emitted HTML. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('exposes the documented generic accessor shape', () => { | ||
| const html = generateDatabrowserHtml('https://x.test/foo', '2.0.0'); | ||
| // window.__dataIsland.get(uri) is the public surface | ||
| assert.match(html, /__dataIsland=window\.__dataIsland\|\|\{get:function/); |
| * `window.__dataIsland.get(uri)` and (when rdflib loads) patches | ||
| * `$rdf.fetcher.load()` to resolve from the inline JSON-LD data island | ||
| * instead of issuing a second HTTP request. These tests pin: | ||
| * - presence in CDN, local, and module HTML wrappers when default |
Comment on lines
+89
to
+91
| */ | ||
| function roundTripOptimizationScript() { | ||
| return `<script>(function(){if(typeof window==='undefined')return;window.__dataIsland=window.__dataIsland||{get:function(uri){if(!uri)return null;try{var esc=window.CSS&&CSS.escape?CSS.escape(uri):String(uri).replace(/["\\\\]/g,'\\\\$&');var el=document.querySelector('script#dataisland[data-uri="'+esc+'"]');if(el&&el.type==='application/ld+json')return{contentType:'application/ld+json',content:el.textContent};}catch(e){}return null;}};var n=0;(function p(){if(++n>100)return;if(typeof $rdf==='undefined'||!$rdf.fetcher||!$rdf.fetcher.load){setTimeout(p,100);return;}if($rdf.fetcher.__dataIslandPatched)return;$rdf.fetcher.__dataIslandPatched=true;var f=$rdf.fetcher,orig=f.load.bind(f);f.load=function(uri,options){var s=(uri&&uri.uri)||(uri&&uri.value)||String(uri);var d=window.__dataIsland.get(s);if(d){return new Promise(function(resolve,reject){$rdf.parse(d.content,f.store,s,d.contentType,function(err){if(err)reject(err);else{f.requested[s]='done';resolve($rdf.sym?$rdf.sym(s):s);}});}).catch(function(){return orig(uri,options);});}return orig(uri,options);};})();})();</script>`; |
- Replace brittle accessor-shape test with public-surface assertion
(no minified-token coupling)
- Fix doc-comment grammar in test header
- Add runtime-behavior suite (9 tests) using Node `vm` to evaluate
the reader script with stubbed window/document/$rdf, pinning:
- accessor returns {contentType, content} on island hit
- accessor returns null on miss / falsy uri
- synchronous patch when rdflib is already present
- patched load resolves from data island, no network call
- patched load falls through to original on miss
- patched load falls through to original on parse error
- patch is idempotent across repeat invocations
- no-rdflib path exits silently, accessor still set up
- Reader: use `window.CSS.escape` instead of bare `CSS.escape`
(consistent with the `window.CSS` guard; works in stricter contexts)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+100
to
+115
| it('reader body does not contain a literal </script> token', () => { | ||
| // The reader is itself a <script> block; any literal end-tag inside | ||
| // its body would terminate the element prematurely. | ||
| const html = generateDatabrowserHtml('https://x.test/foo', '2.0.0'); | ||
| // Extract just the reader script body and check it has no </script> | ||
| // We grep for the IIFE we know is in the reader and assert no end-tag | ||
| // inside the surrounding <script>...</script> the reader uses. | ||
| const readerStart = html.indexOf('window.__dataIsland'); | ||
| assert.ok(readerStart > 0, 'reader script not found'); | ||
| // Walk forward until we find the closing </script> for the reader | ||
| const readerEnd = html.indexOf('</script>', readerStart); | ||
| assert.ok(readerEnd > readerStart, 'reader script not properly closed'); | ||
| const body = html.slice(readerStart, readerEnd); | ||
| assert.doesNotMatch(body, /<\/script>/i, | ||
| 'reader body must not contain </script> token'); | ||
| }); |
Comment on lines
+182
to
+185
| function runReader(ctx, html) { | ||
| vm.runInContext(extractReaderSource(ctx.html || html), ctx); | ||
| } | ||
|
|
| * island, the page renders with one HTTP round-trip instead of two. | ||
| */ | ||
| function roundTripOptimizationScript() { | ||
| return `<script>(function(){if(typeof window==='undefined')return;window.__dataIsland=window.__dataIsland||{get:function(uri){if(!uri)return null;try{var esc=window.CSS&&window.CSS.escape?window.CSS.escape(uri):String(uri).replace(/["\\\\]/g,'\\\\$&');var el=document.querySelector('script#dataisland[data-uri="'+esc+'"]');if(el&&el.type==='application/ld+json')return{contentType:'application/ld+json',content:el.textContent};}catch(e){}return null;}};var n=0;(function p(){if(++n>100)return;if(typeof $rdf==='undefined'||!$rdf.fetcher||!$rdf.fetcher.load){setTimeout(p,100);return;}if($rdf.fetcher.__dataIslandPatched)return;$rdf.fetcher.__dataIslandPatched=true;var f=$rdf.fetcher,orig=f.load.bind(f);f.load=function(uri,options){var s=(uri&&uri.uri)||(uri&&uri.value)||String(uri);var d=window.__dataIsland.get(s);if(d){return new Promise(function(resolve,reject){$rdf.parse(d.content,f.store,s,d.contentType,function(err){if(err)reject(err);else{f.requested[s]='done';resolve($rdf.sym?$rdf.sym(s):s);}});}).catch(function(){return orig(uri,options);});}return orig(uri,options);};})();})();</script>`; |
| * island, the page renders with one HTTP round-trip instead of two. | ||
| */ | ||
| function roundTripOptimizationScript() { | ||
| return `<script>(function(){if(typeof window==='undefined')return;window.__dataIsland=window.__dataIsland||{get:function(uri){if(!uri)return null;try{var esc=window.CSS&&window.CSS.escape?window.CSS.escape(uri):String(uri).replace(/["\\\\]/g,'\\\\$&');var el=document.querySelector('script#dataisland[data-uri="'+esc+'"]');if(el&&el.type==='application/ld+json')return{contentType:'application/ld+json',content:el.textContent};}catch(e){}return null;}};var n=0;(function p(){if(++n>100)return;if(typeof $rdf==='undefined'||!$rdf.fetcher||!$rdf.fetcher.load){setTimeout(p,100);return;}if($rdf.fetcher.__dataIslandPatched)return;$rdf.fetcher.__dataIslandPatched=true;var f=$rdf.fetcher,orig=f.load.bind(f);f.load=function(uri,options){var s=(uri&&uri.uri)||(uri&&uri.value)||String(uri);var d=window.__dataIsland.get(s);if(d){return new Promise(function(resolve,reject){$rdf.parse(d.content,f.store,s,d.contentType,function(err){if(err)reject(err);else{f.requested[s]='done';resolve($rdf.sym?$rdf.sym(s):s);}});}).catch(function(){return orig(uri,options);});}return orig(uri,options);};})();})();</script>`; |
- Reformat reader as multi-line readable JS (was single minified line); easier to maintain and security-review. Browser cost is negligible (~30 lines of inline JS, all in one IIFE). - Close the race where mashlib loads and synchronously calls panes.runDataBrowser() (and hence fetcher.load) before the 100ms poll could fire: install Object.defineProperty getter/setter on window.$rdf so the patch applies the moment $rdf is assigned. Falls back to polling if the property is non-configurable. - Strengthen the </script>-token test: assert exactly one </script> in the reader source (via newly-exported roundTripOptimizationScript) rather than slicing through HTML, where a premature close would have silently passed as the terminator. - Remove dead runReader() helper. - Add two new runtime tests: setter is installed on window.$rdf; setter patches fetcher immediately on assignment. - Make remaining emission-suite regexes whitespace-tolerant since the reader is no longer minified. - Export roundTripOptimizationScript so tests can assert on its source directly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+129
to
+151
| function applyPatch(rdf) { | ||
| if (!rdf || !rdf.fetcher || !rdf.fetcher.load) return; | ||
| if (rdf.fetcher.__dataIslandPatched) return; | ||
| rdf.fetcher.__dataIslandPatched = true; | ||
| var f = rdf.fetcher; | ||
| var orig = f.load.bind(f); | ||
| f.load = function (uri, options) { | ||
| var s = (uri && uri.uri) || (uri && uri.value) || String(uri); | ||
| var d = window.__dataIsland.get(s); | ||
| if (d) { | ||
| return new Promise(function (resolve, reject) { | ||
| rdf.parse(d.content, f.store, s, d.contentType, function (err) { | ||
| if (err) { | ||
| reject(err); | ||
| } else { | ||
| f.requested[s] = 'done'; | ||
| resolve(rdf.sym ? rdf.sym(s) : s); | ||
| } | ||
| }); | ||
| }).catch(function () { return orig(uri, options); }); | ||
| } | ||
| return orig(uri, options); | ||
| }; |
Comment on lines
+178
to
+180
| return vm.createContext({ | ||
| window, document, $rdf, | ||
| setTimeout, clearTimeout, Promise, String, console, |
- Patched fetcher.load() now resolves to a Response-shaped object on data-island hits, matching the orig() path's return type. Consumers that inspect .ok / .status / .url / .headers.get(...) get consistent shape regardless of whether the island was hit or fall-through fired. - Stub setTimeout/clearTimeout in vm test contexts so the reader's polling fallback (~10s of 100ms ticks when $rdf is absent) does not register real Node timers that keep the test process alive past the assertions. Suite duration: 10s → 89ms for the round-trip file; ~10s shaved off full suite. - Extend the data-island-hit runtime test to assert the Response- shaped return contract (ok, status, url, headers.get).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+112
to
+118
| var esc = window.CSS && window.CSS.escape | ||
| ? window.CSS.escape(uri) | ||
| : String(uri).replace(/["\\\\]/g, '\\\\$&'); | ||
| var el = document.querySelector( | ||
| 'script#dataisland[data-uri="' + esc + '"]' | ||
| ); | ||
| if (el && el.type === 'application/ld+json') { |
Comment on lines
+144
to
+155
| f.requested[s] = 'done'; | ||
| // Return a Response-shaped object so consumers that | ||
| // inspect the resolved value (e.g. Response.ok, .status, | ||
| // .url, .headers.get) don't break compared with the | ||
| // original network path. | ||
| resolve({ | ||
| ok: true, | ||
| status: 200, | ||
| statusText: 'OK', | ||
| url: s, | ||
| headers: { get: function () { return null; } } | ||
| }); |
- Reader fast path now resolves to a real `new Response(d.content,
{ status, statusText, headers })` when the constructor is available,
with `url` overridden via Object.defineProperty so consumers reading
it get the resource URL rather than the constructed-Response empty
default. Falls back to a Response-shaped plain object in environments
where the Response constructor is missing. Fixes the shape parity
concern with consumers that use `instanceof Response` or `.text()`.
- Replace CSS-selector construction in `__dataIsland.get()` with
`document.getElementById('dataisland')` + `getAttribute('data-uri')`
string compare. Avoids CSS.escape pitfalls and any selector-injection
surface in older browsers without `CSS.escape`. Single-island-per-page
contract is preserved.
- Guard the success-path callback inside `rdf.parse(...)` with
try/catch and check `f.requested` is an object before assigning, so
unexpected throws (e.g. missing/non-writable `requested` on some
rdflib builds) reject the surrounding Promise instead of leaving it
hanging.
- Update the vm test context: stub `getElementById` instead of
`querySelector`; expose Response in the context. Add three new
runtime tests pinning: real Response return when available,
Response-shaped fallback when not, and graceful resolution when
fetcher.requested is missing (with a 500ms hung-promise guard).
- Fix unintended template-literal early-termination caused by
inline-code backticks in a comment within the reader source.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| status: 200, | ||
| statusText: 'OK', | ||
| url: s, | ||
| headers: { get: function () { return null; } } |
Comment on lines
+108
to
+112
| window.__dataIsland = window.__dataIsland || { | ||
| get: function (uri) { | ||
| if (!uri) return null; | ||
| try { | ||
| // Fetch by id and compare data-uri as a string. Avoids |
- Initialize window.__dataIsland defensively: preserve any pre-existing truthy object but always ensure `.get` is a callable function. Avoids TypeErrors when another script set a partial __dataIsland without `.get`. Existing custom `.get` implementations are not overwritten. - Fallback Response-shaped headers.get() now returns d.contentType for 'content-type' (case-insensitive), null for unknown headers, and null for non-string names. Matches real Response.headers.get() semantics on the inline-data path. - Add three runtime tests pinning: case-insensitive content-type lookup in fallback headers; preserved-but-augmented __dataIsland when prior object lacks .get; non-overwrite of pre-existing custom .get. - Fix recurring backtick-in-comment template-literal early-termination by removing the inline-code backticks from the new defensive-init comment. The repeated return-shape comment from Copilot has been addressed in rounds 3 and 4 (real `new Response()` when available, Response-shaped fallback otherwise, with url overridden via Object.defineProperty).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // window.__dataIsland that lacks a .get function. Preserve any | ||
| // existing object but always ensure .get is callable so consumers | ||
| // never hit a TypeError on the inline-data fast path. | ||
| window.__dataIsland = window.__dataIsland || {}; |
- Strengthen __dataIsland defensive init: explicitly reset to {} when
the existing value is null, undefined, a primitive (string, number,
boolean, symbol), or any other non-object/non-function value.
Previous `||` check let primitives through, so the subsequent .get
assignment would silently fail in non-strict mode (or throw in strict
mode) without ever installing a working accessor.
- Add two runtime tests: a parameterized test exercising string,
number, boolean, and symbol pre-existing values; a separate test
pinning the null case (typeof null === 'object' would defeat a
naive object-only check).
- Fix recurring backtick-in-comment template-literal early-termination
by replacing the inline-code backticks around 'null' with prose.
The repeating return-shape comment from Copilot has been addressed
across rounds 3, 4, and 5: the patched fetcher.load() returns a real
new Response(d.content, ...) when the constructor is available — with
url overridden via Object.defineProperty — and a Response-shaped
plain object (with .ok/.status/.url/.headers.get) as a fallback.
The bot continues to re-emit the original wording but the cited
return values (rdf.sym(s) / s) have not been in the source since
round 3.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Implements the round-trip optimization described in #346. JSS-served HTML wrappers now include a small inline reader script that lets RDF clients resolve
fetcher.load(uri)from the inline JSON-LD data island instead of issuing a second HTTP request. Halves network round-trips for browser views.What changed
src/mashlib/index.js:roundTripOptimizationScript()helper returning the inline<script>blockopts.roundTripOptimizationoption ongenerateDatabrowserHtml()andgenerateModuleDatabrowserHtml()(defaulttrue; passfalseto opt out)test/round-trip-optimization.test.js: 15 new tests covering presence in CDN/local/module modes, opt-out flag, accessor shape, retry-bound, fall-through, and absence of premature</script>close.What the reader does
window.__dataIsland.get(uri)— a generic accessor any client can use, returns{ contentType, content }for a matching data-island element ornull.$rdf.fetcher) loads, patchesfetcher.load()to check the accessor first and parse inline data into the store; falls through to original network fetch on miss or parse error.__dataIslandPatchedflag.Verification
All 571 tests pass (
npm test), including:test/round-trip-optimization.test.jsSide benefits
Closes #346.