fix: guard window.modules access for clay build (esbuild/ESM) compatibility#1571
Closed
fix: guard window.modules access for clay build (esbuild/ESM) compatibility#1571
Conversation
## Why this change exists
Clay Kiln's preloader reads component models and kiln.js files from the
Browserify runtime registry (`window.modules`) via `window.require()`. This
works when the site is built with the legacy `clay compile` (Browserify)
pipeline, which populates `window.modules` at page load.
When the site is built with the new `clay pack-next` (esbuild / ESM) pipeline,
`window.modules` is never defined. Calling `Object.keys(undefined)` throws a
`TypeError` and the entire Kiln preload phase crashes before any component
editing UI is rendered.
## What changed (`lib/preloader/actions.js`)
`getComponentModels()` and `getComponentKilnjs()` both called:
Object.keys(window.modules)
This has been changed to:
Object.keys(window.modules || {})
The `|| {}` short-circuit makes both functions safely return an empty object
when `window.modules` is not present. In pack-next mode this is the correct
behaviour: by the time DOMContentLoaded fires (when Kiln's preload runs),
the `_kiln-edit-init` ESM bundle has already executed and populated
`window.kiln.componentModels` and `window.kiln.componentKilnjs` directly.
The `if (_.isEmpty(...))` guard at the top of each function therefore skips
the `window.modules` loop entirely and uses the already-populated registries.
## Backwards compatibility
No behaviour change for the legacy Browserify build. When `window.modules`
is defined (Browserify build), `window.modules || {}` evaluates to
`window.modules` and the loop runs exactly as before.
Made-with: Cursor
5 tasks
jjpaulino
changed the title
dist/ is gitignored (not published to npm via files field), but when
installing from a GitHub branch npm gets the source without the built
files. Force-commit dist so sites can install directly from
jordan/yolo-update without needing a prepare build step.
Includes the window.modules || {} guard fix in getComponentModels and
getComponentKilnjs so patch-package is no longer needed in sites.
Made-with: Cursor
Made-with: Cursor
- Delete .circleci/ entirely — stops CircleCI from triggering on push - Update build-dist workflow to run on all branches, not just jordan/yolo-update Made-with: Cursor
Replaces all CircleCI jobs with equivalent GitHub Actions workflows: - test.yml — npm test on Node 12 + 14, all branches/tags, Coveralls upload - build-dist.yml — rebuild and commit dist/ after tests pass, all branches - deploy-docs.yml — build website/ and sync to S3 on master push or stg tag - publish.yml — npm publish on version tags; prerelease tag if version has suffix Secrets required (Settings → Secrets → Actions): COVERALLS_REPO_TOKEN, AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, NPM_TOKEN Made-with: Cursor
Made-with: Cursor
node-sass v4 requires a native C++ addon (libsass) that fails to build on Node 20 — no Python, no pre-built binary. It is also end-of-life. Replace with sass (Dart Sass) which is pure JS and needs no compilation. Bump sass-loader from ^6 to ^7 which supports sass as a drop-in without requiring the implementation option. This unblocks npm ci on Node 20 in Docker. Made-with: Cursor
Add a build:dist script (scripts/build-dist.js + git add -f dist/) and register it in the pre-commit hook alongside lint. Every commit on Node <17 now includes the freshly-built dist/ automatically. On Node 17+ (where Webpack 3 hangs), the hook prints a warning and exits cleanly so the commit still lands. The GitHub Actions build-dist workflow (Node 14) remains as the safety net in that case. Remove scripts/build-dist.js once migrated to Vite (roadmap 6.15). Made-with: Cursor
template.js: getComponentName() returns null (not a throw) for bare URIs like 'foo'. Fall back to the uri string itself so componentVariation is never set to null. actions.js: addToQueue().catch() was fire-and-forget — its rejection leaked as an unhandled promise rejection. Node 15+ crashes on these; Node 14 only warned. Return the chain so the outer .catch() handles all errors consistently and the test assertions execute correctly. test.yml: drop Node 12 (EOL Apr 2022, incompatible with sass@^1.79) and remove the redundant Build dist step (pre-commit hook covers it). Made-with: Cursor
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
1 participant
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.
What this PR does
A small defensive fix to
lib/preloader/actions.jsthat makes Clay Kiln's preloader compatible with the newclay build(esbuild + native ESM) pipeline while remaining fully backward-compatible with the legacyclay compile(Browserify) pipeline.Why
Clay Kiln's preloader (
getComponentModelsandgetComponentKilnjs) reads component registrations from the Browserify runtime:This works with
clay compilebecause Browserify populateswindow.modulesat page load.With
clay build(esbuild),window.modulesis never defined — the new pipeline uses native ESM and a generated_kiln-edit-init.jsbundle that populateswindow.kiln.componentModelsandwindow.kiln.componentKilnjsdirectly. CallingObject.keys(undefined)throws aTypeErrorand crashes the entire Kiln preload phase before any component editing UI renders.The fix
The
|| {}short-circuit makes both functions safely return an empty object whenwindow.modulesis absent.Why this is correct for
clay build: by the timeDOMContentLoadedfires (when Kiln's preload runs), the_kiln-edit-initESM bundle has already executed and populatedwindow.kiln.componentModelsandwindow.kiln.componentKilnjs. The existingif (_.isEmpty(...))guard at the top of each function detects the already-populated registries and skips thewindow.modulesloop entirely — so the|| {}path is never actually iterated.Backward compatibility
No behavior change for the legacy Browserify build. When
window.modulesis defined,window.modules || {}evaluates towindow.modulesand the loop runs exactly as before.Related
clay buildin Docker build +resolveMedia.js