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

Conflict with symfony's live components #1316

Open
AdrienClairembault opened this issue Jun 5, 2024 · 4 comments
Open

Conflict with symfony's live components #1316

AdrienClairembault opened this issue Jun 5, 2024 · 4 comments

Comments

@AdrienClairembault
Copy link

  • @testing-library/dom version: 10.1.0
  • Testing Framework and version: Cypress 13.10.0 + @testing-library/cypress 10.0.2
  • App framework: Symfony 7.1.0 (full stack PHP framework)
  • DOM Environment: Any browser

Relevant code or config:

A very simple test case like the one below will break due to some conflicts with the way symfony live components operate.

describe('Synchronize Users', () => {
    it('synchronize user and display a report', () => {
        // Go to the synchronize page
        cy.visit('/admin/synchronize');
        cy.findByRole('region', {
            'name': "Synchronization report"
        }).should('not.exist');

        // Trigger synchronization
        cy.findByRole('button', { name: "Synchronize users" }).click();
        cy.findByRole('region', {
            'name': "Synchronization report"
        });
    });
});

When the testing library attempt to compute the DOM (to display it as debug information), it accidentally trigger mutliple invalid backend requests towards the symfony server:

image

Problem description:

The problem is caused by a feature of a peer dependency: pretty-format.
This dependency is able to recursively pretty print both DOM nodes and complex javacript variables.

To support complex javascript variable, pretty-print has a specific callToJSON feature that will try to call a toJSON method if it exists on ALL items it is recursively iterating on.
I suppose this is mainly useful to support the native implementation Date.prototype.toJSON and allow users to add this method on their objects if need be.

This feature is what leads to the issue, following this code path:

  • @testing-library/dom try to pretty print the DOM container and its children using its prettyDOM() method.
  • The prettyDOM() method then make use of the pretty-format dependency, specifically the format() method.
  • The format() method iterate on the container and its children, using its printComplexValue() method.
  • By doing so, it try to load any properties of the node or object it visits by using its own printObjectProperties() method, with the goal of visiting these properties as potential children.
  • This format()/printObjectProperties() process repeat until we end up iterating over the DOM node of a symfony component, which is where our trouble really begins.
  • The printObjectProperties() method is able to find a specific __component property on our symfony component.
  • This __component property is a Proxy instance that accept any method call (I am not familiar why this specific design is in place, but the calling method name is the action that the component will try to apply to the page).
  • Since it accept any method call, the printComplexValue() method assume that this Proxy support the toJSON method and happily call it.
  • The component accept the method call and understand it as an explicit request to execute the toJSON action.
  • The component query the server to execute this action, which fails as it doesn't exist.

Suggested solution:

The solution proposed in #1315 is to disable the callToJSON feature of the pretty-format dependency using its configuration options.

I am not very familiar with the pretty-format library but it seems very powerful and a lot of its feature are meant to handle complex javascript structures.

Thus, callToJSON is one of these advanced features that seems useless to me if you are only doing DOM pretty printing, which I belive is how it is used by @testing-library/dom (correct me if I'm wrong).

With this in mind, disabling this feature would fix the conflicts with symfony forms without breaking anything as it is (hopefully) irrelevant for DOM printing.

I suppose it could also lead to very small performances improvement as disabling this feature remove a few checks for each DOM node that we iterate over (maybe it could be impactful on huge DOMs ?).

Copy link

github-actions bot commented Jun 5, 2024

Uh oh! @AdrienClairembault, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

@eps1lon
Copy link
Member

eps1lon commented Jun 6, 2024

I think it makes more sense to let users configure the pretty-format options globally. We're going over these again and again and I think there's no good one-size-fits-all. I'd definitely prefer if we call toJson by default since it's not expected that this triggers side-effects. But if a library wants to do that, I'd rather have users explicitly opt-out of it

@MatanBobi
Copy link
Member

I think it makes more sense to let users configure the pretty-format options globally. We're going over these again and again and I think there's no good one-size-fits-all. I'd definitely prefer if we call toJson by default since it's not expected that this triggers side-effects. But if a library wants to do that, I'd rather have users explicitly opt-out of it

This was the approach I was leaning towards too. We can pass option to pretty-print instead of going with a "this" or "that" approach.

@bretrzaun
Copy link

Any news on this? Just stumbled across this issue myself.

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

No branches or pull requests

4 participants