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 ctx transform is identic #77

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

Conversation

milahu
Copy link
Contributor

@milahu milahu commented Dec 21, 2020

replace #75

@milahu
Copy link
Contributor Author

milahu commented Dec 21, 2020

  // TODO why is the initial _path array not empty?
  _path = [createCanvasEvent('beginPath', undefined, {})];

this should probably be empty in the Path2D class
since the method path2d.beginPath does not exist

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 620bbe5 on milahu:add-ctx-transformIsIdentic into c4d1fb2 on hustcc:master.

return { type, transform, props };
return transform
? { type, transform, props }
: { type, props }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again. I try to keep the interface exactly as it is to prevent v8 deoptimization. I don't think this is an appropriate change to jest-canvas-mock.

Copy link
Contributor Author

@milahu milahu Dec 21, 2020

Choose a reason for hiding this comment

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

hmm. make it optional? in my case, i dont use v8 serialize

but then i can post-process the data in my serializer ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its even simpler: we can omit the transform field entirely, cos its 100% redundant
the transform state can always be derived from the past API calls to ctx.setTransform etc

so .. can we add an option to omit the transform field?
(of course in a zero-cost / unswitched way)

Comment on lines 140 to +144
* This array keeps track of the current path, so that fill and stroke operations can store the
* path.
*/
_path = [createCanvasEvent('beginPath', [1, 0, 0, 1, 0, 0], {})];
// TODO why is the initial _path array not empty?
_path = [createCanvasEvent('beginPath', undefined, {})];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtenner can you explain why the _path array is not empty?

at least in the Path2D class, there is no method beginPath
but also an empty context expects me to explicitly call beginPath, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a design choice, and I agree. This probably isn't correct. However, I chose to keep things consistent between CanvasRenderingContext2D class and the Path class. I could be convinced to make a change on the Path2D class, but I'm not entirely sure it would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its helpful to get an *exact* record of API calls, which can be replayed to a live canvas

surely i can work around this issue in my consumer (ignore path.beginPath()), but ..
this *should* be fixed upstream

This was a design choice

why? why should we record calls that were not explicitly made?
again, i expect this tool to produce an *exact* image of my API calls

@SalahAdDin
Copy link

@hustcc is this still active?

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.

4 participants