feat: Add support for unstable_Profiler to mount API #2055

eps1lon · 2019-03-17T19:57:56Z

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

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):

https://github.com/facebook/react/blob/0c03a474362c27aa9222ae27088782a01402067c/packages/react-reconciler/src/ReactFiberCommitWork.js#L564

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

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

package.json

eps1lon · 2019-03-17T22:52:12Z

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 · 2019-03-17T22:56:13Z

<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 · 2019-03-19T08:39:39Z

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 · 2019-03-27T21:18:24Z

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 · 2019-04-03T20:06:56Z

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 · 2019-04-03T20:36:30Z

@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 · 2019-04-03T21:06:10Z

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 · 2019-04-03T21:13:30Z

To clarify, these axioms would ideally remain true:

If it appears in .debug, it is findable
if it is findable, it appears in .debug
if it appears in .debug for mount, it should for shallow
if it appears in .debug for shallow, it should for mount
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 · 2019-04-03T21:31:19Z

Sounds good. Follows principle of least surprise.

ljharb · 2019-04-03T21:34:13Z

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 · 2019-04-05T02:15:33Z

packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js

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


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 🤷‍♂️

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

eps1lon · 2019-04-05T02:48:05Z

And it's green.

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

- [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

holsted · 2019-09-05T20:59:30Z

Is unstable profiler different than the latest Profiler? We're looking for support in our jest tests and are running into issues

ljharb · 2019-09-06T06:02:32Z

@andrewholsted if it's newly renamed in 16.9, enzyme hasn't added support for it yet.

holsted · 2019-09-06T17:35:32Z

@ljharb Ok, thanks. I'm working on a PR now. It seems pretty straight forward (as it's just removing the .unstable_, or keeping it and making it backwards compatible). Running into some issues building enzyme-adapter-react-16 locally and linking to my project to verify. I'm assuming this is a use case enzyme wants to support?

ljharb · 2019-09-06T18:23:35Z

@andrewholsted note that the master branch is failing tests since the 16.9 release, so the first task to unblock all enzyme development is to make a PR fixing that :-/

Yes, the renamed Profiler should definitely be supported.

holsted · 2019-09-07T03:50:29Z

@ljharb - I think this PR fixes all tests on master due to those two issues

#2233

Looks like it passed everything except React 16 on Node 6.

Verified the node 6 failure locally. Will have a look.

[enzyme-adapter-react-16] [new] Add support for wrapping Profiler

a90a845

ljharb reviewed Mar 17, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

ljharb added API: shallow API: mount feature request Issues asking for stuff that would be semver-minor labels Mar 17, 2019

eps1lon mentioned this pull request Mar 18, 2019

fix(backend): Match Profiler display name in react facebook/react-devtools#1316

Closed

eps1lon force-pushed the feat/Profiler branch from 33b59b9 to 260f9ea Compare March 19, 2019 08:30

eps1lon force-pushed the feat/Profiler branch from 260f9ea to 317cb44 Compare March 19, 2019 08:40

eps1lon marked this pull request as ready for review March 19, 2019 08:41

ljharb force-pushed the feat/Profiler branch from 47d30ab to 6588a83 Compare April 3, 2019 20:36

eps1lon force-pushed the feat/Profiler branch 3 times, most recently from 76e401e to 5b4a309 Compare April 5, 2019 02:09

eps1lon commented Apr 5, 2019

View reviewed changes

ljharb force-pushed the feat/Profiler branch from 5203b0a to a90a845 Compare April 5, 2019 05:34

ljharb removed API: mount API: shallow feature request Issues asking for stuff that would be semver-minor labels Apr 5, 2019

ljharb added package: react adapter: 16 semver: minor New stuff. labels Apr 5, 2019

ljharb approved these changes Apr 5, 2019

View reviewed changes

ljharb merged commit a90a845 into enzymejs:master Apr 5, 2019

ljharb mentioned this pull request Apr 5, 2019

support unstable_Profiler #2054

Closed

13 tasks

eps1lon deleted the feat/Profiler branch April 5, 2019 07:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for unstable_Profiler to mount API #2055

feat: Add support for unstable_Profiler to mount API #2055

eps1lon commented Mar 17, 2019 •

edited

Loading

ljharb left a comment

eps1lon commented Mar 17, 2019

ljharb commented Mar 17, 2019

eps1lon commented Mar 19, 2019

eps1lon commented Mar 27, 2019

ljharb commented Apr 3, 2019

ljharb commented Apr 3, 2019

eps1lon commented Apr 3, 2019

ljharb commented Apr 3, 2019

eps1lon commented Apr 3, 2019

ljharb commented Apr 3, 2019

eps1lon Apr 5, 2019

ljharb Apr 5, 2019

eps1lon commented Apr 5, 2019

holsted commented Sep 5, 2019

ljharb commented Sep 6, 2019

holsted commented Sep 6, 2019 •

edited

Loading

ljharb commented Sep 6, 2019

holsted commented Sep 7, 2019 •

edited

Loading

feat: Add support for unstable_Profiler to mount API #2055

feat: Add support for unstable_Profiler to mount API #2055

Conversation

eps1lon commented Mar 17, 2019 • edited Loading

TODO

notes

about find by *

missing timings

prop warnings

ljharb left a comment

Choose a reason for hiding this comment

eps1lon commented Mar 17, 2019

ljharb commented Mar 17, 2019

eps1lon commented Mar 19, 2019

eps1lon commented Mar 27, 2019

ljharb commented Apr 3, 2019

ljharb commented Apr 3, 2019

eps1lon commented Apr 3, 2019

ljharb commented Apr 3, 2019

eps1lon commented Apr 3, 2019

ljharb commented Apr 3, 2019

eps1lon Apr 5, 2019

Choose a reason for hiding this comment

ljharb Apr 5, 2019

Choose a reason for hiding this comment

eps1lon commented Apr 5, 2019

holsted commented Sep 5, 2019

ljharb commented Sep 6, 2019

holsted commented Sep 6, 2019 • edited Loading

ljharb commented Sep 6, 2019

holsted commented Sep 7, 2019 • edited Loading

eps1lon commented Mar 17, 2019 •

edited

Loading

holsted commented Sep 6, 2019 •

edited

Loading

holsted commented Sep 7, 2019 •

edited

Loading