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

Provide access to the "unrendered" element #2544

Open
Andarist opened this issue Nov 12, 2021 · 14 comments
Open

Provide access to the "unrendered" element #2544

Andarist opened this issue Nov 12, 2021 · 14 comments

Comments

@Andarist
Copy link
Contributor

Andarist commented Nov 12, 2021

Is your feature request related to a problem? Please describe.

For serialization purposes (like when working with jest serializer) I'd like to omit some internal elements rendered by component:

const Greeting = ({ children, className }) => (
  <div className={className}>{children}</div>
)
const wrapper = shallow(<Greeting css={{ backgroundColor: 'red' }}>Hello</Greeting>)

expect(wrapper).toMatchSnapshot()

Note that in the context of the serializer plugin I only have access to the ShallowWrapper's instance here. This wrapper renders smth like:

<Fragment>
  <SomethingInternal/>
  <Greeting
    className="emotion-0"
  >
    Hello
  </Greeting>
</Fragment>

And this is what I have direct access to on the wrapper. I would really like to serialize this as:

<Greeting
  className="emotion-0"
>
  Hello
</Greeting>

To avoid handling this in a false-positive manner I really need to know what element has led to this render. This is available as "unrendered" internally but this is not exposed publicly.

Describe the solution you'd like

It would be sufficient if you'd be open to adding a method like unrendered (or similar) to the API.

Describe alternatives you've considered

N/A

Additional context
N/A

@ljharb
Copy link
Member

ljharb commented Nov 12, 2021

I'm not entirely sure I understand.

enzyme doesn't support or endorse snapshot-based testing; it's brittle and leads to folks rubberstamping changes they shouldn't.

The closest we offer is wrapper.debug(). What does that output in this example, and why is that undesirable?

@Andarist
Copy link
Contributor Author

enzyme doesn't support or endorse snapshot-based testing; it's brittle and leads to folks rubberstamping changes they shouldn't.

I agree with this sentiment but some of our (Emotion) users depend on that.

The closest we offer is wrapper.debug(). What does that output in this example, and why is that undesirable?

wrapper.debug() output:

<Fragment>
  <Noop />
  <Greeting className="css-6ph63u-cases">
    Hello
  </Greeting>
</Fragment>

It seems that it is basically stringifying the rendered nodes of the shallow wrapper. I'm after what is called "unrendered" in the codebase here.

I'm not entirely sure I understand.

The problem is that with the upcoming React 18 we must wrap our elements in a Fragment to keep the number of siblings and stuff matching between the server and the client. This avoids useId hydration mismatches in scenarios involving Emotion.

However, this affects serialized snapshots because we add additional elements to the rendered React tree. Those elements are purely internal and ideally, they would be hidden from the serialized snapshots. In the past, this wasn't an issue (even though there was a Fragment there and stuff on the server) because the snapshots are usually captured in the JSDOM environment (so where we previously were avoiding rendering those "internal" elements).

@ljharb
Copy link
Member

ljharb commented Nov 13, 2021

The entire point of snapshot testing is to expose all those internals. Why would it be ideal to provide a fictional snapshot? Doesn't that defeat the entire alleged purpose of having snapshots in the first place?

@ljharb
Copy link
Member

ljharb commented Nov 13, 2021

(also, note that enzyme doesn't yet support react 17 at all yet, so "enzyme and react 18" problems are a very long way off, since all enzyme users are supposed to stay on react 16 until there's official support)

@Andarist
Copy link
Contributor Author

The entire point of snapshot testing is to expose all those internals. Why would it be ideal to provide a fictional snapshot? Doesn't that defeat the entire alleged purpose of having snapshots in the first place?

In this situation, it seems to me that people won't care about the library internals - they should be focused on their code and adding the fragment wrapper etc to the snapshot is just unnecessary noise.

(also, note that enzyme doesn't yet support react 17 at all yet, so "enzyme and react 18" problems are a very long way off, since all enzyme users are supposed to stay on react 16 until there's official support)

We are currently supporting >=16.8.0 || ^17 || ^18. But we don't intend to special-case the runtime code for a specific React version. The stuff that has been added to improve the compatibility with React 18 works just fine with React 16 and React 17. So it's not as much about React 18 as it is about hiding those "internal" elements from the snapshot. Note that this is already possible for any element within the rendered tree - but the "unrendered" element is not a part of it, so the "root" cannot be handled in the same way. I could implement some heuristics to detect this situation but it would be brittle and possibly could result in false positives. That's why I would be interested in having a first-class API to check what has been used to render the given enzyme wrapper.

While my use case might not be that appealing to you, this could be used for different use cases as well. I'm not asking about something specific to the whole serialization stuff that I'm dealing with - it's just some background on why I'm requesting it.

I understand that you might be hesitant about adding this - just trying to make a case for myself here 😉

@ljharb
Copy link
Member

ljharb commented Nov 14, 2021

Totally fine to ask :-) I'm still just trying to understand the value.

The root element is the thing you're testing. Since you're testing it, you are the author of it, you already know all the internal implementation details, and all of those details are relevant. It sounds like this is a scenario where someone's making a snapshot of a component they didn't author, which is confusing to me.

@Andarist
Copy link
Contributor Author

It sounds like this is a scenario where someone's making a snapshot of a component they didn't author, which is confusing to me.

This is the case here. With smth like this:

/** @jsx jsx */

<div css={{ color: 'red'}} />

the user is thinking about the div and its styles - those are the details of the implementation that they care about. Because a css prop is used here and thanks to a custom JSX pragma, in reality, a wrapper component is being rendered here though that renders that div under the hood.

The content of the wrapper component is irrelevant to the user though and, IMHO, there would be little to no value in printing it in the snapshot.

@ljharb
Copy link
Member

ljharb commented Nov 25, 2021

A custom jsx pragma means you’re not using react rendering - which means you’d need a custom enzyme adapter. The react adapters only work with the react jsx transforms.

@Andarist
Copy link
Contributor Author

Emotion is using jsx pragma with React - we forward calls to React.createElement:
https://github.com/emotion-js/emotion/blob/f046ae40bcae24400068311690a94ba2dbf20344/packages/react/src/jsx.js

So we dont quite need custom enzyme adapters at all.

@ljharb
Copy link
Member

ljharb commented Nov 25, 2021

right - but emotion's transform does different things than react's does, which is why you're seeing undesirable results in the enzyme debug tree. The only reason you wouldn't need a custom adapter is if the custom transform didn't do anything, which would defeat its purpose.

@Andarist
Copy link
Contributor Author

Hm, I'm not sure if I totally understand the point here. Maybe we could provide a custom adapter and thus gain control over this. It seems like a lot of work for a very little gain though - and also a problem for the users because we'd have to educate them to use that custom adapter over the "default" one. Emotion works OK with the regular adapter just fine - functional-wise. There is just this minor issue with the serialization output. We can actually handle this by using some heuristics - I would just prefer not to rely on guessing there and to be able to implement a straightforward logic.

@ljharb
Copy link
Member

ljharb commented Nov 25, 2021

Right, I do see the difficulty you're in.

I suppose we could come up with a way to configure the standard adapters with a custom jsx transform - but then your users would still need to configure their adapters with something like import { emotionEnzyme } from 'emotion'; enzyme.configure({ adapter, ...emotionEnzyme });.

Certainly changing the default tree representation wouldn't be feasible. Exposing the internal UNRENDERED value is doable, but I'm very hesitant to do it prior to having working React 17 and 18 adapters, because i have no idea what kinds of internal changes will be needed for enzyme to support react 18.

@Andarist
Copy link
Contributor Author

Well, I've raised this mostly because I had a need for this and this would make my life slightly easier. It isn't something I'm desperate for (I've also implemented an ugly, ugly hack to make this work for me) so I'm OK with this just sitting around for now.

Certainly changing the default tree representation wouldn't be feasible. Exposing the internal UNRENDERED value is doable, but I'm very hesitant to do it prior to having working React 17 and 18 adapters, because i have no idea what kinds of internal changes will be needed for enzyme to support react 18.

Fair game. If you ever get to work on those you might have this ticket back in your head - if you forget about it, that is fine too 😉

@ljharb
Copy link
Member

ljharb commented Nov 26, 2021

Sounds good :-) Happy to leave this open, and we can revisit it once there's a React 17 and 18 adapter.

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

2 participants