Dedupe install across bundled copies; pin the ordering invariant - #8
Open
myabc wants to merge 1 commit into
Open
Dedupe install across bundled copies; pin the ordering invariant#8myabc wants to merge 1 commit into
myabc wants to merge 1 commit into
Conversation
installElements guarded against double-install only within one module
instance: the installed flag and the blessings.includes identity check
both fail when the package is bundled twice (two dependency graphs),
so each copy pushed its own ElementsBlessing.
Tag the blessing with Symbol.for("@openproject/stimulus-elements.
blessing") and dedupe on the tag: the global symbol registry is
shared across module copies, so any copy recognises a blessing
installed by another.
Also pin the README's ordering invariant with a characterization
test: controllers registered before installElements() silently lack
accessors — Stimulus snapshots blessings at registration onto a
shadow constructor and leaves no observable trace, so a late-install
warning is not implementable without an import-time side effect
(forbidden by sideEffects: false). Documented in README and install.ts.
There was a problem hiding this comment.
Pull request overview
This PR hardens installElements() by making blessing installation dedupe across multiple bundled copies of the library (so the “double bundle” case becomes safe by behavior, not just by comment), and it pins the Stimulus ordering invariant (“install before register/start”) with focused browser-mode tests and clearer README guidance.
Changes:
- Tag the blessing with a
Symbol.for(...)key and dedupe based on that tag rather than function identity. - Add dedicated tests covering (a) silent failure when controllers are registered before install and (b) cross-bundle dedupe behavior.
- Update documentation and add a patch changeset describing the behavior and invariant.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/install-order.test.ts | Adds an end-to-end characterization test for the “install before register/start” ordering invariant and its silent failure mode. |
| test/install-dedupe.test.ts | Adds a test ensuring installElements() dedupes a blessing installed by another bundled copy via Symbol.for tagging. |
| src/install.ts | Implements cross-bundle blessing dedupe using a shared symbol tag and documents the ordering invariant. |
| README.md | Documents that late install fails silently and that repeated installs (including bundled duplicates) are safe. |
| .changeset/calm-installs-dedupe.md | Adds a patch changeset for the cross-bundle dedupe + documented silent failure invariant. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+24
to
+30
| // silent failure: no accessors, no warning, no error | ||
| expect(early.thingElement).toBeUndefined() | ||
| expect(early.hasThingElement).toBeUndefined() | ||
|
|
||
| // installing afterwards does not retro-bless already-registered controllers | ||
| installElements() | ||
| expect(early.thingElement).toBeUndefined() |
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.
Ticket
N/A — internal architecture review of this library (candidate 5).
What are you trying to accomplish?
Give
installElements()its invariant. The README's hard requirement — install beforeregister()/Application.start()— was unenforced, untested, and fails silently (accessors justundefined). Separately, the double-install guard only worked within one module instance:installedflag andblessings.includesidentity check both fail when the package is bundled twice, so each copy pushed its own blessing — a caveat living only in a comment.What approach did you choose and why?
Symbol.for("@openproject/stimulus-elements.blessing")and dedupe on the tag. The global symbol registry is shared across module copies, so any copy recognises a blessing installed by another. The comment-caveat becomes behavior.Controller.blessingsreads from module-eval time, i.e. an import-time side effect, whichsideEffects: falseforbids. Insteadtest/install-order.test.tscharacterizes the silent failure end-to-end (early controller lacks accessors, install doesn't retro-bless, late controller works) — it regression-guards the failure mode and catches any Stimulus behavior change. README now states the failure is silent.integration.test.ts'sbeforeEach(installElements)would otherwise destroy.Patch changeset included. CONTEXT.md decision entry deferred — the file arrives with #1; this PR is independent of that chain.
Stacked on #6 (vitest browser mode) only; independent of the #1 ← #2 ← #4 chain.
Verification:
bun run test35 pass in Chromium,bun run buildclean.Merge checklist
Lookbook docsn/a — npm library; README updated