Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: should prefix ShadowRoot with window. #2943

Merged
merged 1 commit into from
Feb 3, 2021

Conversation

sodatea
Copy link
Member

@sodatea sodatea commented Jan 5, 2021

Otherwise this expression would throw in environments that do not
support ShadowRoot, including the common mocha testing environment
setup that uses jsdom and jsdom-global.

It is because ShadowRoot is not an enumerable property on window,
jsdom-global fails to expose it on the global object.

See the error message at https://app.circleci.com/pipelines/github/vuejs/vue-cli/779/workflows/17d7d7c4-7605-4588-878a-ddb3a6d37102/jobs/24147

Otherwise this expression would throw in environments that does not
support `ShadowRoot` which includes the common mocha testing environment
setup that uses `jsdom` and `jsdom-global`.

It is because `ShadowRoot` is not an enumerable property on `window`,
`jsdom-global` fails to expose it on the `global` object.

See the error message at: https://app.circleci.com/pipelines/github/vuejs/vue-cli/779/workflows/17d7d7c4-7605-4588-878a-ddb3a6d37102/jobs/24147
sodatea added a commit to sodatea/vue-cli that referenced this pull request Jan 6, 2021
@LinusBorg LinusBorg added 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. ready to merge The PR is ready to be merged. labels Jan 6, 2021
@sodatea sodatea mentioned this pull request Jan 15, 2021
@mattelen
Copy link

Hi everyone, any thoughts on when this will be merged in master? Quite keen for this bugfix. TIA

@LinusBorg LinusBorg added this to Planned in Next Patch Feb 1, 2021
@LinusBorg LinusBorg moved this from Planned to Approved in Next Patch Feb 1, 2021
@LinusBorg LinusBorg merged commit 97d6f1a into vuejs:master Feb 3, 2021
Next Patch automation moved this from Approved to Done Feb 3, 2021
@yyx990803 yyx990803 moved this from Done to Final (Reviewed by Evan) in Next Patch Feb 24, 2021
@jjdive
Copy link

jjdive commented Mar 23, 2021

Hi everyone!

I'm running into an error related to this when trying to run a simple test using typescript. I've come up with a minimum repo to reproduce this case at https://github.com/jjdive/vite-vue-ts-tests-error. Anyways, this is the error i get and it points to if (container instanceof window.ShadowRoot && at normalizeContainer (node_modules/@vue/runtime-dom/dist/runtime-dom.cjs.js:1288:19).

Thanks in advance for any help with this!

image

@sodatea
Copy link
Member Author

sodatea commented Mar 23, 2021

@jjdive I'm on my phone so I can't test it. But here's my guess:
It's because your jest version is outdated. Jest 24 uses jsdom v11 by default https://github.com/facebook/jest/blob/v24.0.0/packages/jest-environment-jsdom/package.json#L14 But Shadow DOM support was added in jsdom v12 jsdom/jsdom#2347

@sodatea
Copy link
Member Author

sodatea commented Mar 23, 2021

You can either update your jest version or change the configuration to use a different testEnvironment, like what we do in Vue CLI v4 https://github.com/vuejs/vue-cli/blob/b0de229e78ae860c6ebc1e53f4bb481d7549cde7/packages/%40vue/cli-plugin-unit-jest/presets/default/jest-preset.js#L21

@jjdive
Copy link

jjdive commented Mar 23, 2021

@sodatea, you're totally right! vue-jest@5 is asking for ts-jest@24 (as per 5.0.0-alpha.7) which in turn requires jest@24, so i had to update with:

npm i -D jest@26 --legacy-peer-deps

and the sample test runs without issues (i know this might lead to errors at some point so i'll be looking forward to any vue-jest and the other deps updates).

Thank you very much for pointing me to the right dep update needed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. ready to merge The PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants