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

add prop to check instead of trying to delete, which fails in Jsdom 21+ #2058

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

emn-dev
Copy link

@emn-dev emn-dev commented Dec 19, 2023

Description

Since Jsdom 21 the check for canvas doesn't work correctly when running on the server (non-browser) environment. We are not allowed to delete the window prop from the Jsdom object anymore.

Using Jsdom 20 it works = https://stackblitz.com/edit/stackblitz-starters-acumcu?file=index.ts
Using Jsdom 21 it is broken = https://stackblitz.com/edit/stackblitz-starters-lx596o?file=index.ts
Jsdom changelog = https://github.com/jsdom/jsdom/blob/main/Changelog.md#2100
Jsdom code = jsdom/jsdom@0c8eb3a
Line 480 in Window.js file make the window prop configurable=false, and that means we can't delete it.

Related issues

None that I know of, but I haven't looked deeply.

Checklist

  • New tests added or existing tests modified to cover all changes (This isn't so easy to get running :( )
  • Code conforms with the JSHint rules (yarn run jshint passes)

@sapics
Copy link
Member

sapics commented Sep 25, 2024

Thanks for making PR! And so sorry for late response.

I know little about jsdom, thus, it is difficult for me to review the PR.

By the way, we use jsdom v16.
Dose it mean that we need to apply your PR when we update jsdom over v21, or we need to apply ASAP?

@lehni
Copy link
Member

lehni commented Sep 25, 2024

I'd like to spend some time soon to update the dependencies. We need to address this, but I'm not sure this PR is the right way to go about it.

@sapics
Copy link
Member

sapics commented Sep 25, 2024

@lehni Thank you for your comment! Updating dependencies sounds great!

@emn-dev
Copy link
Author

emn-dev commented Sep 27, 2024

I'd like to spend some time soon to update the dependencies. We need to address this, but I'm not sure this PR is the right way to go about it.

@lehni , I have another pr over here = #2083, that is a bump I needed to get canvas to work on my mac. I would be happy to do multiple prs and update in chunks. E.g. bump yarn to v4.5.0 first. Then move on to other dependencies, e.g. Jsdom. Deal with any errors or tests breaking for one or very few dependencies at a time.

Happy to help :)

Thoughts?

@emn-dev
Copy link
Author

emn-dev commented Sep 30, 2024

Thanks for making PR! And so sorry for late response.

I know little about jsdom, thus, it is difficult for me to review the PR.

By the way, we use jsdom v16. Dose it mean that we need to apply your PR when we update jsdom over v21, or we need to apply ASAP?

This pr is targeted enough that it could be done ASAP. If we only wanted to bump JsDom that should be very doable. I would have to check on the tests again but I don't think there was any problem with latest JsDom. Currently JsDom is at version 25.x https://github.com/jsdom/jsdom/releases

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