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

Replace canvas dependency in@jspsych/config with jest-canvas-mock #3488

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

jodeleeuw
Copy link
Member

No description provided.

Copy link

changeset-bot bot commented Jan 14, 2025

🦋 Changeset detected

Latest commit: f438099

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@jspsych/config Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jodeleeuw jodeleeuw requested a review from bjoluc January 14, 2025 16:02
Copy link
Member

@bjoluc bjoluc left a comment

Choose a reason for hiding this comment

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

Yes, thanks! 🎉 👍 No need to bother people outside the core repo with the canvas dependency. Don't forget to commit the updated package-lock.json 🙂

.changeset/little-roses-kick.md Show resolved Hide resolved
.changeset/little-roses-kick.md Outdated Show resolved Hide resolved
@bjoluc
Copy link
Member

bjoluc commented Jan 14, 2025

Maybe this is also a chance to move to jest-canvas-mock in the corresponding packages? I'm in favor of abandoning the canvas dependency altogether, considering all the build issues it has caused in the past 😅

@jodeleeuw
Copy link
Member Author

ooh I like that idea very much!

@bjoluc
Copy link
Member

bjoluc commented Jan 14, 2025

I would've expected the tests to fail since we haven't set up the mocking with jest yet. I guess canvas is still in the lock file. Time (which I should not take right now) for some NPM debugging again; too bad that we haven't made the switch to pnpm yet 🥲

@jodeleeuw
Copy link
Member Author

Yeah, that's what I figured too. Got it to break and now it is working again. I ended up replacing the dependency in @jspsych/config with jest-canvas-mock since the easiest way to implement it was to add it to the jest config setupFiles. Doesn't seem like a problem to leave it in @jspsych/config now that it doesn't require the build steps that canvas did. WDYT?

@jodeleeuw
Copy link
Member Author

OK well now tsc is failing and I'm not sure why. Will get back to this in a bit...

@jodeleeuw jodeleeuw changed the title Move canvas dependency from @jspsych/config to packages that use canvas Replace canvas dependency in@jspsych/config with jest-canvas-mock Jan 14, 2025
@jodeleeuw
Copy link
Member Author

OK, tsc was broken because typescript somehow reverted to version 3.x in the package-lock.json despite being specified as 5.2.2. Updating to 5.7.3 fixed that. Now the build is broken on GH but OK locally 🥴

@jodeleeuw jodeleeuw requested a review from bjoluc January 14, 2025 21:21
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.

2 participants