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

Show functions and not <Profiler/> with react 16.3 #1793

Merged
merged 3 commits into from
Aug 29, 2018

Conversation

lukeapage
Copy link
Contributor

@lukeapage lukeapage commented Aug 27, 2018

We haven't quite yet upgraded to react 16.4 and with 16.3 Profiler is not defined, meaning that any react nodes without a type (like a function) in shallow get serialized as <Profiler/>

Its not that important but I thought it could happen in the future, so I did a small refactor to not match undefined and undefined.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks - could you add a test case that would fail without your changes?

case AsyncMode: return 'AsyncMode';
case Fragment: return 'Fragment';
case StrictMode: return 'StrictMode';
case Profiler: return 'Profiler';
Copy link
Member

Choose a reason for hiding this comment

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

i wonder if a simpler change, since this is the 16.3 adapter, would be to just delete the Profiler line?

@ljharb
Copy link
Member

ljharb commented Aug 27, 2018

We'll also need the same change in the 16 adapter, since that still supports 16.0 - 16.4.

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

hmm, we have an explicit test that covers function children showing up as [function]. Can you share your test case?

@lukeapage
Copy link
Contributor Author

I'm a bit confused now.

  1. The Profiler line is valid for 16.4, which I thought the 16.3 adapter supported?
  2. I can do || NaN as a simpler change as per your commit, but then I may as well keep the code as-is - should I simplify it?

I don't have a (unit) test case at the moment but I can have a go at writing one once I understand what changes you want.

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

@lukeapage the 16.3 adapter does not support 16.4, only the 16 adapter does. I've already pushed up commits that use || NaN.

Specifically I'm interested in reproducing:

meaning that any react nodes without a type (like a function) in shallow get serialized as

from your OP. Can you provide a case that does that in React 16.3?

@lukeapage
Copy link
Contributor Author

I followed the instructions and I get errors running anything in enzyme

$ npm run env -- 16

> [email protected] env C:\git\enzyme
> babel-node ./env.js "16"

events.js:183
      throw er; // Unhandled 'error' event
      ^

Error: spawn npm ENOENT
    at _errnoException (util.js:992:11)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:190:19)
    at onErrorNT (internal/child_process.js:372:16)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] env: `babel-node ./env.js "16"`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] env script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

on node 8.11.4

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

@lukeapage it's entirely possible that our dev scripts don't work on windows; could you share the code itself, and i can try to repro it?

@lukeapage
Copy link
Contributor Author

I did add a unit test - it will fail because I don't know exactly what the function output is.

its a class that passes a function callback, shallow rendered and passed to jest's toMatchSnapshot

class B extends React.Component {
   render() {
       this.props.children();
   }
}
class A extends React.Component {
    render() {
       return <B>{() => <div/>}</B>;
    }
}

const wrapper = shallow(<A></A>);
expect(wrapper).toMatchSnapshot();

and its the snapshot serializer which uses the display name during serialization.

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

Thanks, I'll give that a shot.

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

What's the "snapshot serializer" here? enzyme doesn't have snapshot support - only .debug(), which i'd recommend instead - and it seems like your test case is passing just fine on master in React 16.3.

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

I'm going to merge this, since i still think these are good fixes - but I'm pretty confident your issue is that you're relying on non-enzyme snapshotting. If you stick with .debug() output, you should be fine.

@ljharb ljharb merged commit 5be7999 into enzymejs:master Aug 29, 2018
ljharb added a commit that referenced this pull request Aug 30, 2018
 - [fix] `debug`: prevent unavailable React types from matching `undefined` types (#1793)
ljharb added a commit that referenced this pull request Aug 31, 2018
 - [fix] `debug`: prevent unavailable React types from matching `undefined` types (#1793)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants