Skip to content
This repository has been archived by the owner on May 16, 2024. It is now read-only.

Filter invalid matchers before extending expect #16

Merged
merged 2 commits into from
Mar 18, 2022
Merged

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Mar 17, 2022

Change Type

Closes #15

This fixes @storybook/jest for vite projects, by removing the __esmodule and default properties (and anything else that isn't a valid matcher type, or at least that's undefined or boolean) before extending expect.

I've tested this in my own vite project by copying the build file into my node_modules, and it worked as expected. It would be good to double-check in a webpack project as well, though.

This should only be needed until/unless testing-library/jest-dom#438 is released and can be used.

Indicate the type of change your pull request is:

  • maintenance
  • documentation
  • patch
  • minor
  • major
📦 Published PR as canary version: 0.0.10--canary.16.6472401.0

✨ Test out this PR locally via:

npm install @storybook/[email protected]
# or 
yarn add @storybook/[email protected]

@IanVS IanVS requested a review from yannbf March 17, 2022 21:29
@yannbf
Copy link
Member

yannbf commented Mar 17, 2022

Awesome stuff @IanVS ! Thanks a lot, seems reasonable to me. I don't think this can break in webpack apps but I'll test it out tomorrow!

@yannbf
Copy link
Member

yannbf commented Mar 18, 2022

LGTM @IanVS! I'm just wondering about the effects of the change:

Seems like the original matchers are only evaluated once they are accessed via their getters:
image
image

And the validMatchers from this PR already are fully evaluated because Object.assign will have to read the getters in order to copy the values over.
image

I guess that just means a very slight performance impact, which I'm fine with

@ghengeveld
Copy link
Member

Generally good change, but I'd rather keep the original getter behavior. So instead of spreading matchers onto a new object, just mutate matchers by deleting unwanted properties directly.

@IanVS
Copy link
Member Author

IanVS commented Mar 18, 2022

just mutate matchers by deleting unwanted properties directly.

Unfortunately that's not possible, which is why I had to spread into a new object. Trying to delete the default property results in an error:

Imports are immutable in JavaScript. To modify the value of this import, you must export a setter
  function in the imported file (e.g. "setDefault") and then import and call that function here
  instead.

@IanVS
Copy link
Member Author

IanVS commented Mar 18, 2022

Also note, I'm hoping this change is only temporary until testing-library/jest-dom#438 can be released, which will avoid this problem by using real esm instead of this unfortunate bundler behavior.

@yannbf yannbf added the patch Increment the patch version when merged label Mar 18, 2022
@yannbf yannbf self-assigned this Mar 18, 2022
@yannbf
Copy link
Member

yannbf commented Mar 18, 2022

I added a reminder in the code to undo that once the other PR is done. Thank you so much @IanVS for pushing Vite forward and sticking with us to make this first class support!

@yannbf yannbf merged commit e4b9acc into main Mar 18, 2022
@yannbf yannbf deleted the 15-extend-default branch March 18, 2022 13:16
@github-actions
Copy link

🚀 PR was released in v0.0.10 🚀

@github-actions github-actions bot added the released This issue/pull request has been released. label Mar 18, 2022
@IanVS IanVS mentioned this pull request Aug 23, 2023
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
patch Increment the patch version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] extended matchers does not work storybook-builder-vite as-is
4 participants