Skip to content

Ship highlight.js verbatim from @highlightjs/cdn-assets#112

Open
kellenmurphy wants to merge 1 commit into
simplesamlphp:mainfrom
kellenmurphy:fix/hljs-cdn-assets
Open

Ship highlight.js verbatim from @highlightjs/cdn-assets#112
kellenmurphy wants to merge 1 commit into
simplesamlphp:mainfrom
kellenmurphy:fix/hljs-cdn-assets

Conversation

@kellenmurphy

Copy link
Copy Markdown
Contributor

AMO's review of 1.9.4 is stuck on reproducing the minification of lib/highlight.min.js, and I don't think that's winnable while we ship a custom build. There's no official artifact to checksum against, so the reviewer is forced to reproduce a build, and ours was never reproducible in the first place: package.json pinned highlight.js to a bare GitHub URL with no ref, the workflow re-resolved it with npm install --package-lock-only before clean-install, npm audit fix ran immediately before the build, and package-lock.json was export-ignored out of the release archive. Nothing in the submitted zip identified a fixed version. Their build also died at git rev-parse --short=10 HEAD because they unpacked the source zip rather than cloning — hljs embeds the commit hash in the banner, so corrected instructions wouldn't have helped either.

So this consumes the project's own published distribution instead, pinned exactly, copied without modification. @highlightjs/cdn-assets is highlight.js' own npm package rather than a third-party CDN rehost, which matters for the "official site" rule — package-lock.json records the tarball URL and its SHA-512 integrity hash, so a reviewer verifies the shipped files with npm ci and diff and never has to build anything. I've also stopped export-ignoring the lock-file for that reason, and dropped --package-lock-only and npm audit fix from the workflow, since both mutate the dependency tree at build time and reintroduce exactly the problem we're trying to fix.

The es/ modules give core plus only the xml, http and properties grammars, about 24 KB, slightly smaller than the custom build was. src/hljs-init.js is the only new first-party code: it imports those four files, registers the grammars and exposes hljs as a global so the page's classic scripts are untouched. It has to be a module script, but that's not a timing hazard — every hljs call site is event-driven, nothing runs at load. I verified in a loaded extension that window.hljs is defined, that exactly xml, http and properties are registered, that versionString is 11.11.1, and that highlightElement still emits hljs- classes with no console errors.

One wrinkle worth flagging: cdn-assets has no 11.11.1 successor published yet, so this pins one patch behind the 11.11.2 the lock-file previously resolved to. I think a documentable downgrade beats an unreproducible build, but it's your call.

Refs #109.

AMO's review of 1.9.4 is blocked on reproducing the minification of
lib/highlight.min.js. That is unwinnable as long as we ship a custom build:
there is no official artifact to checksum against, so the reviewer has to
reproduce a build, and reproducing ours was never possible anyway --
package.json pinned highlight.js to a bare GitHub URL with no ref, the
workflow re-resolved it with `npm install --package-lock-only` before
clean-install, `npm audit fix` ran immediately before the build, and
package-lock.json was stripped from the release archive. Nothing in the
submitted zip identified a fixed version.

Consume the project's own published distribution instead, pinned exactly, and
copy the files without modification. @highlightjs/cdn-assets is highlight.js'
own npm package rather than a third-party CDN rehost, so package-lock.json
records the tarball URL and its SHA-512 integrity hash: a reviewer verifies
the shipped files with `npm ci` and `diff`, with no build to reproduce.

The es/ modules give core plus only the xml, http and properties grammars --
about 24 KB, slightly smaller than the custom build. A first-party shim
registers them and exposes hljs as a global so the page's classic scripts are
unchanged; every hljs call site is event-driven, so the deferred module
script is not a timing hazard.

cdn-assets has no 11.11.1 successor yet, so this pins one patch behind the
11.11.2 the lock-file previously resolved to. A documentable downgrade is
worth more than an unreproducible build.

Refs simplesamlphp#109
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants