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

feat: Add support for unstable_Profiler to mount API #2055

Merged
merged 1 commit into from Apr 5, 2019

Conversation

2 participants
@eps1lon
Copy link
Contributor

eps1lon commented Mar 17, 2019

Part of #2054

Opening this with failing tests. Will need to establish if we accept those for now or fix before merge. I'm also missing some context what work in mount is done by React and what is re-implemented in this package.

TODO

  1. revert pretest removal
  2. add find by display name support
  3. add find by id support
  4. investigate missing timings
  5. prop warnings for Profiler

notes

about find by *

I guess this is related to Profiler not appearing in wrapper.debug() output. Might be caused by Profiler being one of those "mode-components" (e.g. StrictMode or ConcurrentMode) Solved and explained in #2055 (comment)

missing timings

These are implented in the fiber related code (ReactFiberCommitWork):

onRender isn't even called with ReactTestRenderer.create. Maybe this is the actual blocker here?
Update: Timings weren't missing. The test environment didn't have a performance API available at which point React uses Date.now which isn't precise enough to display any duration.

prop warnings

There is currently a test case that included a bare <Profiler /> (existed before this PR). Those should trigger warnings about missing id and onRender props. Probably never triggered since this is not actual prop types validation but happens when creating the fiber: https://github.com/facebook/react/blob/4186952a6f3558eb4fae9f6c5f669bd898dc1d97/packages/react-reconciler/src/ReactFiber.js#L597
Update: Warnings are logged perfectly fine with mount. I don't think we need to shim those in getDisplayName.

@ljharb
Copy link
Member

ljharb left a comment

  • should the "mode" components exist in .debug()? ideally yes, but we'd want it to be the same answer for mount and shallow, and react may have made the decision for us.
  • if it does not show up in debug, what happens when you mount or shallow a Profiler itself, instead of just a component that renders one? (because the root wrapper needs to wrap something, and mount shows that something in its debug tree)
  • The prop warnings seems useful to shim in whenever the Profiler is used
  • There very well might be a bug with the test renderer, or with server rendering - ideally the profiler has actual meaningful behavior (even if that's "it just noops and renders its children) on the server, and thus in the shallow renderer
Show resolved Hide resolved package.json Outdated
@eps1lon

This comment has been minimized.

Copy link
Contributor Author

eps1lon commented Mar 17, 2019

should the "mode" components exist in .debug()? ideally yes, but we'd want it to be the same answer for mount and shallow, and react may have made the decision for us.

It does appear in the react-devtools browser extension and Reacts component stack traces which is why I thought it should appear in .debug() as well. But if the debug output should correspond to shallow levels? then it should match those.

I also had find in mind (currently 2 skipped test cases). I don't know if there are use cases to find a certain Profiler.

There very well might be a bug with the test renderer, or with server rendering - ideally the profiler has actual meaningful behavior (even if that's "it just noops and renders its children) on the server, and thus in the shallow renderer

Added some more test cases to the codesandbox in #2054. It looks like for ReactTestRenderer.create onRender is never called. I guess mount should behave similar?

Going to look into this again and open an issue on the react repo. It's not obvious to me how the Profiler should behave in react-test-renderer.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 17, 2019

<Profiler> is a "shallow level"; i think that means it must appear in debug. You definitely should be able to .find(Profiler) if you like, altho I can't conceive of why you would :-)

The first source of truth is React itself in a browser - this determines how mount works. Next, we look at React on the server (ReactDOM.renderToString). This determines how SSR works. shallow tries to mimic mount as much as possible, and only deviates where a DOM is required, and possibly based on how SSR works. So, if react-test-renderer doesn't do something that React does, and there's an argument to be made that it should, then it's a bug with the test renderer.

@eps1lon

This comment has been minimized.

Copy link
Contributor Author

eps1lon commented Mar 19, 2019

Update:
Timings weren't missing. The test environment didn't have a performance API available at which point React uses Date.now which isn't precise enough to display any duration.

Warnings are logged perfectly fine with mount. I don't think we need to shim those in getDisplayName.

react-test-renderer/shallow currently throws if you pass a valid Profiler component. I asked in the profiler RFC what the plans are for react-test-renderer. I guess until then it's ok for the Profiler to not appear in .debug() or be available in .find().

@eps1lon eps1lon force-pushed the eps1lon:feat/Profiler branch from 260f9ea to 317cb44 Mar 19, 2019

@eps1lon eps1lon marked this pull request as ready for review Mar 19, 2019

@ljharb ljharb added this to v16.6+: memo, lazy, suspense in React 16 Mar 27, 2019

@eps1lon

This comment has been minimized.

Copy link
Contributor Author

eps1lon commented Mar 27, 2019

Anything blocking this? The purpose is only to support mount in this PR. shallow support requires the feature flag to be enabled in React. Once this is released we can revisit the shallow API. It would be unfortunate if this would block partial support of the profiler in enzyme.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 3, 2019

Given that the implication is that it would work in the shallow renderer once it becomes stable, it seems like we'd want to add (skipped) shallow tests now, as well.

@ljharb ljharb force-pushed the eps1lon:feat/Profiler branch from 47d30ab to 6588a83 Apr 3, 2019

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 3, 2019

@eps1lon ok, i've rebased this and tweaked some things. Some tests are now failing for me locally, only because Profile is appearing in the shallow renderer - for consistency with mount, I think either we need to fix mount so that Profiler does show up in .debug - or, we need to fix shallow so that it does not (and update both the shallow and mount tests to consistently reflect that reality).

@eps1lon

This comment has been minimized.

Copy link
Contributor Author

eps1lon commented Apr 3, 2019

I would include it in the debug() output since it would be closer to the actual component stack (in react and devtools). If it appears in debug() it should probably be findable too?

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 3, 2019

To clarify, these axioms would ideally remain true:

  1. If it appears in .debug, it is findable
  2. if it is findable, it appears in .debug
  3. if it appears in .debug for mount, it should for shallow
  4. if it appears in .debug for shallow, it should for mount
  5. if the react dev tools show something, it should show up in .debug (if the react dev tools do not show something, we can still choose to include it, if it passes the other criteria)

Thoughts?

@eps1lon

This comment has been minimized.

Copy link
Contributor Author

eps1lon commented Apr 3, 2019

Sounds good. Follows principle of least surprise.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 3, 2019

To be even more explicit :-) once the debug/findability/shallow-mount-consistency issue is resolved, I'm stoked to merge and release this. The currently-skipped shallow tests, pending changes in the shallow renderer, don't need to block full support for mount (as long as we have a path to identical support in the future, which we do)

@eps1lon eps1lon force-pushed the eps1lon:feat/Profiler branch 3 times, most recently from 76e401e to 5b4a309 Apr 5, 2019

@@ -207,6 +207,7 @@ function toTree(vnode) {
case FiberTags.ContextProvider:
case FiberTags.ContextConsumer:
return childrenToTree(node.child);
case FiberTags.Profiler:

This comment has been minimized.

Copy link
@eps1lon

eps1lon Apr 5, 2019

Author Contributor

This was the mistake. I previously included this in the same branch as the context tags, strict mode etc. That branch will not be considered a node. At least this is how I understood it.

There's probably an argument to be made that they should also be considered as valid nodes (since they also appear in reacts component stack and devtools) but that's for another time (personally I'm not interested in those components WRT to find/debug).

Not sure if Profiler should be handled exactly like forwardRef but tests are passing for now so 🤷‍♂️

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 5, 2019

Member

hmm - if they appear in react devtools, and the stack, then they definitely need to appear in debug.

@eps1lon

This comment has been minimized.

Copy link
Contributor Author

eps1lon commented Apr 5, 2019

And it's green.

shallow and mount now create the same component tree WRT to the Profiler.

@ljharb ljharb force-pushed the eps1lon:feat/Profiler branch from 5203b0a to a90a845 Apr 5, 2019

@ljharb

ljharb approved these changes Apr 5, 2019

@ljharb ljharb merged commit a90a845 into airbnb:master Apr 5, 2019

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 91.671%
Details

@ljharb ljharb referenced this pull request Apr 5, 2019

Closed

support unstable_Profiler #2054

3 of 13 tasks complete

@eps1lon eps1lon deleted the eps1lon:feat/Profiler branch Apr 5, 2019

ljharb added a commit that referenced this pull request Apr 6, 2019

[enzyme-adapter-react-16] v1.12.0
 - [new] Add support for wrapping `Profiler` (#2055)
 - [new] support shallow rendering `createContext()` providers and consumers  - add `isContextConsumer`, `getProviderFromConsumer` (#1966)
 - [new] add `wrapWithWrappingComponent`, `isCustomComponent` (#1960)
 - [new] add `getDerivedStateFromError` support (#2036)
 - [fix] avoid invariant violation in provider (#2083)
 - [fix] properly fix finding memo(SFC) components (#2081)
 - [fix] properly render memoized SFCs
 - [fix] `shallow`: avoid wrapping component for hooks
 - [deps] update `react-is`
 - [dev deps] update `eslint`
 - [refactor] use `react-is` predicates more
 - [build] include source maps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.