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.

Sign up for GitHub

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

Jump to bottom

getComputedStyle cache issue with v21.1.0 #3502

Closed
cexbrayat opened this issue Jan 27, 2023 · 5 comments
Closed

getComputedStyle cache issue with v21.1.0 #3502

cexbrayat opened this issue Jan 27, 2023 · 5 comments

Comments

@cexbrayat
Copy link

cexbrayat commented Jan 27, 2023 •
edited
Loading

Basic info:

  • Node.js version: v16.14.2
  • jsdom version: 21.1.0

Minimal reproduction case

IIUC jsdom v21.1 introduced caching for getComputedStyle, triggering test failures when we upgraded to it.

Please take a look at the following test.

It accesses the computed style of an element created with document.createElement but never inserts it in the DOM, then updates it, then accesses the computed style again.

Then the test fails because the computed style is cached and we get the same one as the first access.
Removing the first access makes the test succeed (or downgrading to v21.0).

See the repro online on the following stackblitz if you prefer
https://stackblitz.com/edit/vitest-dev-vitest-whharw?file=test/basic.test.ts

const { JSDOM } = require('jsdom');

const { window } = new JSDOM(`<!DOCTYPE html><body></body>`);
const element = window.document.createElement('span');
// document.querySelector('body').appendChild(element);

console.log('before', element.style.display); // ''
// commenting out this line makes the test succeed
console.log(window.getComputedStyle(element).display); // ''

element.style.display = 'none';

console.log('after', element.style.display); // 'none'
console.log(window.getComputedStyle(element).display); // '' instead of 'none'

The test succeeds if:

  • we comment the first check
  • we use jsdom v21.0
  • we insert the element in the DOM
@cexbrayat cexbrayat mentioned this issue Jan 27, 2023
Merged
1 task
@cexbrayat cexbrayat reopened this Jan 27, 2023
@cexbrayat
Copy link
Author

cexbrayat commented Jan 27, 2023 •
edited
Loading

EDIT: I updated the original repro with Vue code (as the one without was not reproducing the issue)

@domenic
Copy link
Member

domenic commented Jan 27, 2023

Closing per this comment in the issue template:

Please create a minimal repro. Any reports involving third party libraries
will be closed, as we cannot debug third-party library interactions for you.

Please do not use syntax that is not supported in Node.js, such as JSX. If
we cannot run the code in Node.js, we will close the issue, as we cannot
debug whatever toolchain you are using.

TO BE CLEAR: your example needs to be self-contained enough that we can
copy-paste it into a file named test.js, and then run it using
node test.js. No Jest or similar.

IF YOU DO NOT FOLLOW THESE INSTRUCTIONS WE WILL CLOSE THE ISSUE.

Let us know if you can follow these instructions, and then we can reopen and investigate the issue.

@domenic domenic closed this as completed Jan 27, 2023
@cexbrayat
Copy link
Author

@domenic Sorry about that, I updated the repro 👍

@domenic domenic reopened this Jan 28, 2023
cexbrayat added a commit to cexbrayat/vue-test-utils-next that referenced this issue Jan 28, 2023
@cexbrayat
fix: use attachTo is failing tests with jsdom v21.1
946da56 
`attachTo` can be removed when jsdom/jsdom#3502 is fixed
 With jsdom v21.1, `getComputedStyle` is cached and not updated for elements not the DOM
 so some our tests fail (because the elements are checked a first time)
cexbrayat added a commit to cexbrayat/vue-test-utils-next that referenced this issue Jan 28, 2023
@cexbrayat
fix: use attachTo is failing tests with jsdom v21.1
5aef13b 
`attachTo` can be removed when jsdom/jsdom#3502 is fixed
 With jsdom v21.1, `getComputedStyle` is cached and not updated for elements not in the DOM
 so some our tests fail (because the elements are checked a first time)
@cexbrayat cexbrayat mentioned this issue Jan 28, 2023
Merged
cexbrayat added a commit to cexbrayat/vue-test-utils-next that referenced this issue Jan 28, 2023
@cexbrayat
fix: use attachTo in failing tests with jsdom v21.1
b1fb6a6 
`attachTo` can be removed when jsdom/jsdom#3502 is fixed
 With jsdom v21.1, `getComputedStyle` is cached and not updated for elements not in the DOM
 so some our tests fail (because the elements are checked a first time)
lmiller1990 pushed a commit to vuejs/test-utils that referenced this issue Jan 31, 2023
@cexbrayat
fix: use attachTo in failing tests with jsdom v21.1 (#1952)
6c90ff7 
`attachTo` can be removed when jsdom/jsdom#3502 is fixed
 With jsdom v21.1, `getComputedStyle` is cached and not updated for elements not in the DOM
 so some our tests fail (because the elements are checked a first time)
@domenic
Copy link
Member

domenic commented Feb 12, 2023

Thanks for updating the repro, and sorry for the delay as I was on vacation.

It looks to me like jsdom's behavior now correctly matches browsers: https://jsbin.com/covuqujaru/edit?html,console . Do you agree? If so, then I think this is working as intended, and you got a bugfix when you upgraded jsdom.

@cexbrayat
Copy link
Author

Thanks for taking a look @domenic If that now matches the browser's behavior then I can't argue

For a bit of context: this triggers regressions in the Vue ecosystem as the official testing lib does not attach tested components to the DOM by default. We'll probably have to change that.

Thanks for your time

@ikex ikex mentioned this issue Apr 6, 2023
Closed
alecgibson added a commit to alecgibson/test-utils that referenced this issue Jul 5, 2023
@alecgibson
docs: add warning about isVisible()
50fbcfc 
This change updates the documentation to reflect that `isVisible()`
will only work correctly [when attached to the DOM][1] because of an
upstream [`jsdom` issue][2].

[1]: vuejs#2016
[2]: jsdom/jsdom#3502
@alecgibson alecgibson mentioned this issue Jul 5, 2023
Merged
cexbrayat pushed a commit to vuejs/test-utils that referenced this issue Jul 5, 2023
@alecgibson @cexbrayat
docs: add warning about isVisible()
f0df231 
This change updates the documentation to reflect that `isVisible()`
will only work correctly [when attached to the DOM][1] because of an
upstream [`jsdom` issue][2].

[1]: #2016
[2]: jsdom/jsdom#3502
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@cexbrayat @domenic