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

build(deps): update canvas to 2.8.0 #1367

Merged
merged 5 commits into from
Sep 14, 2021

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Sep 9, 2021

I've updated canvas dependency to 2.8.0 to fix a missing function Automattic/node-canvas#1769 and I've removed jest-canvas-mock because canvas already mocks everything we need.

@monfera I've also tried the failing changes in https://github.com/elastic/elastic-charts/pull/1309/files replacing the clearCanvas method with:

export function clearCanvas(ctx: CanvasRenderingContext2D) {
  const { a, d } = ctx.getTransform().invertSelf().scale(ctx.canvas.width, ctx.canvas.height);
  ctx.clearRect(0, 0, a, d);
}

and it works correctly now

@markov00 markov00 added dependencies Pull requests that update a dependency file :build Build tools / dependencies skip-newsletter labels Sep 9, 2021
@nickofthyme
Copy link
Collaborator

Are there any cases where we would not want the full implementation and instead just want to mock out any canvas elements in unit tests?

@markov00
Copy link
Member Author

markov00 commented Sep 9, 2021

Are there any cases where we would not want the full implementation and instead just want to mock out any canvas elements in unit tests?

Probably in most cases, but the think is that the jest-canvas-mock doesn't mocks everything correctly see hustcc/jest-canvas-mock#86 and hustcc/jest-canvas-mock#85 so we can switch it off completely.
The other benefit from the canvas only library is that we can "rely" on a real implementation for the textMeasure API in canvas, where the mock just return the string length.

@markov00
Copy link
Member Author

markov00 commented Sep 10, 2021

jenkins test this

retrying due to failure on linking a c++ lib required by canvas https://kibana-ci.elastic.co/job/elastic+elastic-charts+pull-request/2146/

@markov00
Copy link
Member Author

markov00 commented Sep 10, 2021

There are two possible solutions to the above-mentioned issue in Jenkins:

@markov00
Copy link
Member Author

markov00 commented Sep 11, 2021

jenkins test this
the last run was successful without changes: https://kibana-ci.elastic.co/job/elastic+elastic-charts+pull-request/2150/

@nickofthyme
Copy link
Collaborator

@markov00 It seems to be using the canvas on the VRT which it should not

04:46:47  FAIL  integration/tests/interactions.test.ts
04:47:39   ● Test suite failed to run
04:47:39 
04:47:39     /lib64/libstdc++.so.6: version `CXXABI_1.3.9' not found (required by /var/lib/jenkins/workspace/elastic+elastic-charts+pr-vrts/node_modules/canvas/build/Release/canvas.node)
04:47:39 
04:47:39       at Runtime._loadModule (../node_modules/jest-runtime/build/index.js:893:29)
04:47:39       at Object.<anonymous> (../node_modules/canvas/lib/bindings.js:3:18)

@nickofthyme nickofthyme reopened this Sep 14, 2021
@nickofthyme nickofthyme merged commit bf1a3ef into elastic:master Sep 14, 2021
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 36.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:build Build tools / dependencies dependencies Pull requests that update a dependency file released Issue released publicly skip-newsletter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants