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

Pass window object to mockPrototype #100

Merged
merged 1 commit into from
May 18, 2023

Conversation

jdufresne
Copy link
Contributor

When mocking the canvas on a non-global window object, the prototype
mocks should apply to the same window object.

@hustcc
Copy link
Owner

hustcc commented Oct 21, 2022

Can it work? do your have tested it?

When mocking the canvas on a non-global window object, the prototype
mocks should apply to the same window object.
@jdufresne
Copy link
Contributor Author

@hustcc I added a test case in the latest revision. Is this what you had in mind?

@hustcc hustcc merged commit ca41794 into hustcc:master May 18, 2023
@virtuoushub
Copy link
Contributor

observation

I am running into an issue when upgrading to 2.5.1 and I believe it has something to due with this change.

...
client:test: TypeError: Cannot read properties of undefined (reading 'prototype')
client:test:  ❯ mockPrototype ../../node_modules/.pnpm/jest-canvas-mock@2.5.1/node_modules/jest-canvas-mock/lib/mock/prototype.js:37:50
client:test:  ❯ Proxy._default ../../node_modules/.pnpm/jest-canvas-mock@2.5.1/node_modules/jest-canvas-mock/lib/window.js:61:26
client:test:  ❯ src/setupTests.ts:31:22
client:test:      [29](<code-under-test>/actions/runs/5090740096/jobs/9149933044?pr=1141#step:8:30)| ] as const;
client:test:      [30](<code-under-test>/actions/runs/5090740096/jobs/9149933044?pr=1141#step:8:31)| 
client:test:      [31](<code-under-test>/actions/runs/5090740096/jobs/9149933044?pr=1141#step:8:32)| const canvasWindow = getCanvasWindow({ document: window.document });
client:test:        |                      ^
client:test:      [32](<code-under-test>/actions/runs/5090740096/jobs/9149933044?pr=1141#step:8:33)| 
client:test:      [33](<code-under-test>/actions/runs/5090740096/jobs/9149933044?pr=1141#step:8:34)| apis.forEach((api) => {
...

looking at the test case, it only covers when there is a wellformed window object.

Not sure what the expected behavior when something other than a wellformed window object is; however I was not seeing the above error in 2.5.0 and below. There seems to some delta between the global objects (HTMLCanvasElement, etc.) and this newly passed window object.

currently failing line: https://github.com/hustcc/jest-canvas-mock/blob/master/src/mock/prototype.js#L37

...
  win.HTMLCanvasElement.prototype.getContext = getContext2D;
...

suggestion

suggestion - update the code to have this change be more conditional where we check if the passed in win object has the necessary props to not error in functions like getContext2D, and if it does not; fall back to the old behavior ( mockPrototype() ).

drawbacks, this potentially might be masking something in a project where the user expects the browser to behave one way; and due to all this mocking abstraction it behaves differently during testing. I think this is acceptable since, we are already basically doing this now, it just will behave slightly differently with the proposed suggestion than it does now, so I wanted to call it out.

@jdufresne jdufresne deleted the mock-prototype-window branch May 26, 2023 13:51
@hustcc
Copy link
Owner

hustcc commented May 29, 2023

Maybe I should revert the commit, @virtuoushub can help me do this?

@virtuoushub
Copy link
Contributor

Maybe I should revert the commit, @virtuoushub can help me do this?

I don't think a revert is necessary, let me see if I can come up with a slightly modified implementation using my suggestion.

fwiw, I opened a new issue over at #115

@virtuoushub
Copy link
Contributor

Maybe I should revert the commit, @virtuoushub can help me do this?

opened up a fix over at #116

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.

None yet

3 participants