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

Disable callToJSON option used by pretty-format #1315

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AdrienClairembault
Copy link

What:

This PR disable the callToJSON option used by the pretty-format dependency.

Why:

These changes are needed to fix a conflict that happens when trying to add E2E tests to a project that uses Symfony's live components.

The issue is caused by the process of building the "pretty printed" DOM debug data.
This process is handled by the pretty-format dependency, which will by default call a toJSON method on any object that defines it.

See the relevant from pretty-format:

image

This seems harmless most of the time but conflict with symfony's live component because the component DOM node is a proxied object which support any method being called on it (which will be interpreted as an "action").

For more context, here is the relevant code used by the proxy:

image

With this in mind, the typeof val.toJSON === 'function' check of pretty-format will be validated and it will call toJSON on symfony DOM items, which will trigger a backend request for a toJSON action, which does not exist, thus breaking all tests.

How:

Luckily pretty-format has a option which can be used to disable the whole toJSON logic.

The testing library use only uses pretty-format to display DOM items which will never support this special method unless they are weirdly proxied.
Disabling the option should thus be fairly safe and have no negative impact (the test suite seems to confirm it as all tests passes without issues).

Checklist:

  • Documentation added to the docs site N/A
  • Tests N/A (not sure how this could be properly tested as it relies on an external framework internal behavior. I tried adding a case with a custom proxy to simulate the issue but it didn't really make sense in the end.)
  • TypeScript definitions updated N/A
  • Ready to be merged

Copy link

codesandbox-ci bot commented Jun 5, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 59b21a5:

Sandbox Source
react-testing-library-examples Configuration

@MatanBobi
Copy link
Member

MatanBobi commented Jun 5, 2024

Thanks @AdrienClairembault. Can you please open an issue with a reproduction of the problem so we'll understand what we're trying to fix here with a real life example? Also, I'm not entirely sure we'd want to merge this change as this looks like the default behavior that fits most use cases.

@AdrienClairembault
Copy link
Author

AdrienClairembault commented Jun 5, 2024

I have opened an issue which hopefully should be a bit clearer.

Also, I'm not entirely sure we'd want to merge this change as this looks like the default behavior that fits most use cases.

I completely understand this and very much respect that you would not blindly change a configuration option and risk potential side effects.

My point of view is that the change would be safe as the option is not relevant when doing DOM nodes comparison but javascript is not my main field at all and I would appreciate if someone with more experience on the subject could confirm it.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Let's discuss this first: #1316

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.

3 participants