Skip to content

[CDX-404] Update packages and jsdom version#268

Merged
esezen merged 1 commit intomainfrom
cdx-404-autocomplete-ui-review-and-merge-open-dependabot-prs
Apr 13, 2026
Merged

[CDX-404] Update packages and jsdom version#268
esezen merged 1 commit intomainfrom
cdx-404-autocomplete-ui-review-and-merge-open-dependabot-prs

Conversation

@esezen
Copy link
Copy Markdown
Contributor

@esezen esezen commented Mar 18, 2026

Ran npm audit fix to get rid of security vulnerabilities. I had to upgrade jsdom to version 30 which is a breaking change with major differences.

  • Upgrade jest-environment-jsdom from ^29.7.0 to ^30.3.0 (pulls in jsdom@26)
  • Fix all 18 test files that relied on Object.defineProperty(window, 'location', ...) which is no longer allowed in jsdom 26
  • Add a custom jsdom environment to patch jsdom's navigation behavior

Why do we need a custom jsdom environment?

jsdom 26 does not support window.location.href = '...' — it throws "Not implemented: navigation (except hash changes)". This is a long-standing jsdom limitation, but in previous versions our tests worked around it by replacing window.location entirely with a plain URL object (where .href is just a writable property, not a navigation trigger).

With jsdom 26 locking down window.location, we can no longer swap it out. Our source code (urlHelpers.ts, shopifyDefaults.ts) intentionally assigns to window.location.href to navigate, and multiple tests exercise that code path. Without the patch, those tests would fail with the "Not implemented: navigation" error.

Let me know if there is an easier approach to this 🤔

@esezen esezen requested a review from Mudaafi as a code owner March 18, 2026 18:28
Copilot AI review requested due to automatic review settings March 18, 2026 18:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the test/runtime dependency set to address npm audit findings, including moving the Jest JSDOM environment forward (bringing in a newer jsdom) and adjusting tests/environment behavior to accommodate jsdom’s stricter window.location handling.

Changes:

  • Upgrade jest-environment-jsdom to ^30.3.0 (pulling in jsdom@26.x) and update lockfile accordingly.
  • Add a custom Jest environment module that patches jsdom navigation behavior for window.location.href assignments.
  • Refactor shopifyDefaults tests to stop replacing window.location and instead set window.location.href directly.

Reviewed changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
spec/utils/shopifyDefaults.test.ts Updates tests to avoid redefining window.location and use window.location.href instead.
spec/jest-env-jsdom.js Adds a custom environment side-effect patch to make window.location.href = ... update the document URL rather than throw.
jest.config.js Switches the client project to use the custom jsdom environment module.
package.json Bumps jest-environment-jsdom version.
package-lock.json Updates dependency graph for the new Jest/jsdom environment version.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread package.json
Comment thread package.json
Comment thread spec/jest-env-jsdom.js
Comment thread spec/jest-env-jsdom.js
Comment thread spec/jest-env-jsdom.js
@@ -0,0 +1,12 @@
const JSDOMEnvironment = require('jest-environment-jsdom').default;
@constructor-claude-bedrock
Copy link
Copy Markdown

Code Review Results

✅ Strengths

The approach of replacing the unsafe Object.defineProperty(window, 'location', ...) hack in tests with a real window.location.href assignment is a genuine improvement in test fidelity, and the PR description clearly documents the jsdom 26 limitation and the chosen solution.

🚨 Critical Issues

  • [File: spec/jest-env-jsdom.js Line: L6-L9] The patch monkey-patches a private internal API of jsdom (LocationImpl.prototype._locationObjectNavigate and _relevantDocument._URL) that is not part of any public contract. Internal implementation details prefixed with _ can be renamed or removed in any minor or patch release without a semver breaking-change notice, silently breaking tests. If jsdom renames _locationObjectNavigate or _URL, the patch will silently do nothing and tests will fail with the "Not implemented: navigation" error again. A safety assertion should be added at module load time, e.g.:

    if (typeof LocationImpl.prototype._locationObjectNavigate !== 'function') {
      throw new Error(
        'jest-env-jsdom.js: jsdom internal API changed — update the Location patch'
      );
    }
  • [File: package.json Line: L103] jest is pinned at ^29.7.0 while jest-environment-jsdom is now ^30.3.0. This intentional cross-major-version combination works because jest-environment-jsdom v30 bundles its own copies of @jest/environment and jest-mock, but it is an unusual setup that could confuse future maintainers into thinking jest itself is at v30. Add a comment or a note in the PR description documenting this intentional mismatch and why jest was not upgraded alongside the environment.

⚠️ Important Issues

  • [File: spec/jest-env-jsdom.js Line: L8-L10] The patched _locationObjectNavigate updates _relevantDocument._URL but does not trigger navigation side-effects such as popstate events or window.history updates. Tests currently only assert on window.location.href, so this is fine today. However, any future test that checks window.history.length or registers a popstate listener will silently get incorrect results. This limitation should be documented in a comment on these lines.

  • [File: spec/utils/shopifyDefaults.test.ts Line: L11] There is no afterEach to restore window.location.href. The suite correctly relies on beforeEach to reset the URL before each test, but the test at line L128 sets a mid-test URL divergence and cleanup depends entirely on execution order. Adding afterEach(() => { window.location.href = initialUrl; }) would make isolation explicit and guard against future test ordering changes.

💡 Suggestions

  • [File: spec/jest-env-jsdom.js Line: L5] The comment "This must run before any JSDOM instance is created (module-level)" is accurate but the deeper reason — that LocationImpl.prototype is shared across all jsdom instances — should be stated explicitly to help future maintainers understand whether the patch needs re-application per instance.

  • [General] The PR description mentions "Fix all 18 test files that relied on Object.defineProperty(window, 'location', ...)" but the diff shows only one test file changed (shopifyDefaults.test.ts). Clarifying in the PR description whether the other 17 files never used that pattern, were handled in a prior commit, or are out of scope would help reviewers understand the full impact.

Overall Assessment: ⚠️ Needs Work

Copy link
Copy Markdown
Contributor

@TarekAlQaddy TarekAlQaddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we have to keep in mind the jsdom internal patching just in case it ever changes in future versions

@esezen esezen merged commit 5d9f5ef into main Apr 13, 2026
15 checks passed
@esezen esezen deleted the cdx-404-autocomplete-ui-review-and-merge-open-dependabot-prs branch April 13, 2026 17:25
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.

3 participants