fix(core): initialise _dom outside the GUI block so the core build can run - #829
Open
okadriu wants to merge 1 commit into
Open
fix(core): initialise _dom outside the GUI block so the core build can run#829okadriu wants to merge 1 commit into
okadriu wants to merge 1 commit into
Conversation
…n run
The core build strips //{{START: GUI}} blocks, which removed the _dom
initialiser while leaving two unconditional writes to it in place:
config-init.js sets _dom._document and general.js sets
_dom._serviceCheckboxInputs[category]. run() therefore threw immediately
in dist/core.
Moving the initialiser out of the GUI block keeps the full build
byte-identical and makes the core build usable.
|
@okadriu is attempting to deploy a commit to the Orest Bida's projects Team on Vercel. A member of the Team first needs to authorize it. |
✅ Deploy Preview for cookieconsentv3-playground canceled.
|
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.
The
dist/corebuild throws as soon asrun()is called, so it cannot be used at all:What happens
GlobalStateinitialisesthis._dominside a//{{START: GUI}} ... //{{END: GUI}}block insrc/core/global.js, sorollup-core.config.mjsstrips it. Two writes to that same object sit outside any GUI block and survive into the core bundle:src/core/config-init.js:29sets_dom._document = doc;src/utils/general.js:191setsglobalObj._dom._serviceCheckboxInputs[categoryName] = {};The first one throws immediately. Fixing only that surfaces the second right behind it, with
TypeError: Cannot set properties of undefined (setting 'necessary'), so both maps need to exist in the core build as well.The change
Move the
this._dom = { ... }initialiser out of the GUI block. The two maps stay declared and are simply never populated in the core build, which is what their readers already handle:src/utils/general.js:428-429both fall back with|| {}.Verification
I built both bundles from source at
8596c79, once before and once after the change.dist/corerun()dist/cookieconsent.esm.jsThe full build coming out byte-identical was the part I most wanted to confirm, since it means existing users cannot be affected.
I then exercised the whole exported API of the fixed core build in Node, with a minimal
documentandnavigatorshim (no DOM needed, it is the UI-less build):run,acceptCategory,acceptedCategory,acceptService,acceptedService,getCookie,getConfig,getUserPreferences,validConsent,validCookie,setCookieData,eraseCookiesandreset, plus three lifecycles that matter in practice: reset followed by a freshrun(), a reload with an existing consent cookie, andopt-outmode. All 22 steps pass after the change; four of them throw before it.Finally I dropped the rebuilt core into a real Next.js application that uses this library for its logic with a custom React interface, and ran the full consent flow in a browser: first visit, accept all, withdraw, decide again. Cookies, categories, revision and the consent record all behaved correctly, with no console errors.
pnpm teststill reports 119 passed across 9 suites.Notes
This touches
src/only. I leftdist/for you to regenerate at release time, matching what #825 did.The core build has no test coverage at the moment, which is presumably why this went unnoticed. I can add a smoke test that imports
dist/coreafter a build step, either here or as a follow-up, but I did not want to change your test setup unasked.I ran into this while using the library for its logic with a custom React interface on top.
dist/coreis exactly the right artifact for that, which is why fixing it seemed more useful than working around it.