Skip to content

expose devtools hook with react-reconciler package #11445

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

Closed

Conversation

iamdustan
Copy link
Contributor

[react] yarn build react-reconciler
yarn run v1.2.1
$ npm run version-check && node scripts/rollup/build.js react-reconciler

> @16.1.0-beta version-check /Users/iamdustan/projects/react/react
> node ./scripts/tasks/version-check.js

 BUILDING  react-reconciler.development.js (node_dev)
 COMPLETE  react-reconciler.development.js (node_dev)

 BUILDING  react-reconciler.production.min.js (node_prod)
 COMPLETE  react-reconciler.production.min.js (node_prod)

 BUILDING  react-reconciler-devtools.development.js (node_dev)
 COMPLETE  react-reconciler-devtools.development.js (node_dev)

 BUILDING  react-reconciler-devtools.production.min.js (node_prod)
 COMPLETE  react-reconciler-devtools.production.min.js (node_prod)

┌─────────────────────────────────────────────────────────┬───────────┬──────────────┬─────────────┬───────────┬──────────────┬─────────────┐
│ Bundle                                                  │ Prev Size │ Current Size │ Diff        │ Prev Gzip │ Current Gzip │ Diff        │
├─────────────────────────────────────────────────────────┼───────────┼──────────────┼─────────────┼───────────┼──────────────┼─────────────┤
│ react-reconciler.development.js (NODE_DEV)              │ 258.73 KB │ 258.73 KB    │ 0 %         │ 53.2 KB   │ 53.2 KB      │ 0 %         │
├─────────────────────────────────────────────────────────┼───────────┼──────────────┼─────────────┼───────────┼──────────────┼─────────────┤
│ react-reconciler.production.min.js (NODE_PROD)          │ 39.63 KB  │ 39.63 KB     │ 0 %         │ 12.32 KB  │ 12.32 KB     │ 0 %         │
├─────────────────────────────────────────────────────────┼───────────┼──────────────┼─────────────┼───────────┼──────────────┼─────────────┤
│ react-reconciler-devtools.development.js (NODE_DEV)     │ 0 B       │ 2.83 KB      │ +Infinity % │ 0 B       │ 1.05 KB      │ +Infinity % │
├─────────────────────────────────────────────────────────┼───────────┼──────────────┼─────────────┼───────────┼──────────────┼─────────────┤
│ react-reconciler-devtools.production.min.js (NODE_PROD) │ 0 B       │ 831 B        │ +Infinity % │ 0 B       │ 485 B        │ +Infinity % │
└─────────────────────────────────────────────────────────┴───────────┴──────────────┴─────────────┴───────────┴──────────────┴─────────────┘

was working on updating a renderer to the official API and realized I had lost access to this. I think should be relatively noncontroversial and safe. The ReactDevtoolsHook file has only one real import from fbjs/lib/warning and hten the type imports from ReactFiber and ReactFiberRoot.

cc @gaearon @bvaughn

[react] yarn build react-reconciler
yarn run v1.2.1
$ npm run version-check && node scripts/rollup/build.js react-reconciler

> @16.1.0-beta version-check /Users/iamdustan/projects/react/react
> node ./scripts/tasks/version-check.js

 BUILDING  react-reconciler.development.js (node_dev)
 COMPLETE  react-reconciler.development.js (node_dev)

 BUILDING  react-reconciler.production.min.js (node_prod)
 COMPLETE  react-reconciler.production.min.js (node_prod)

 BUILDING  react-reconciler-devtools.development.js (node_dev)
 COMPLETE  react-reconciler-devtools.development.js (node_dev)

 BUILDING  react-reconciler-devtools.production.min.js (node_prod)
 COMPLETE  react-reconciler-devtools.production.min.js (node_prod)

┌─────────────────────────────────────────────────────────┬───────────┬──────────────┬─────────────┬───────────┬──────────────┬─────────────┐
│ Bundle                                                  │ Prev Size │ Current Size │ Diff        │ Prev Gzip │ Current Gzip │ Diff        │
├─────────────────────────────────────────────────────────┼───────────┼──────────────┼─────────────┼───────────┼──────────────┼─────────────┤
│ react-reconciler.development.js (NODE_DEV)              │ 258.73 KB │ 258.73 KB    │ 0 %         │ 53.2 KB   │ 53.2 KB      │ 0 %         │
├─────────────────────────────────────────────────────────┼───────────┼──────────────┼─────────────┼───────────┼──────────────┼─────────────┤
│ react-reconciler.production.min.js (NODE_PROD)          │ 39.63 KB  │ 39.63 KB     │ 0 %         │ 12.32 KB  │ 12.32 KB     │ 0 %         │
├─────────────────────────────────────────────────────────┼───────────┼──────────────┼─────────────┼───────────┼──────────────┼─────────────┤
│ react-reconciler-devtools.development.js (NODE_DEV)     │ 0 B       │ 2.83 KB      │ +Infinity % │ 0 B       │ 1.05 KB      │ +Infinity % │
├─────────────────────────────────────────────────────────┼───────────┼──────────────┼─────────────┼───────────┼──────────────┼─────────────┤
│ react-reconciler-devtools.production.min.js (NODE_PROD) │ 0 B       │ 831 B        │ +Infinity % │ 0 B       │ 485 B        │ +Infinity % │
└─────────────────────────────────────────────────────────┴───────────┴──────────────┴─────────────┴───────────┴──────────────┴─────────────┘
@gaearon
Copy link
Collaborator

gaearon commented Nov 3, 2017

Can you also fix all files importing it directly to use this form please?

@iamdustan
Copy link
Contributor Author

iamdustan commented Nov 3, 2017

Would that be a diff that basically looks like this?

 import * as ReactPortal from 'react-reconciler/src/ReactPortal';
-import {injectInternals} from 'react-reconciler/src/ReactFiberDevToolsHook';
+import injectInternals from 'react-reconciler/devtools';
 import ReactGenericBatching from 'events/ReactGenericBatching';

(not super familiar with the lerna/yarn workspaces expectations)

Should I keep it as importing the named injectInternals (current master) or is having just the one export okay?

@gaearon
Copy link
Collaborator

gaearon commented Nov 3, 2017

I think we need them all to be exported. Because scheduler needs to be able to call on* functions without invasive imports too.

@gaearon
Copy link
Collaborator

gaearon commented Nov 3, 2017

It makes sense to keep injectInternals as default export though.

@gaearon
Copy link
Collaborator

gaearon commented Nov 5, 2017

Hmm. Seems like we have the same problem as with the renderers here. We can't use a singleton, can we? Since then two renderers would conflict.

@gaearon
Copy link
Collaborator

gaearon commented Nov 5, 2017

I'm also not convinced this does the right thing for third party renderers. Have you tested it?

This would set functions like onCommitFiberRoot in the /devtools bundle. But the reconciler is going to look for them (and not find them) in its own copy of this file.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

This approach wouldn't work as is (see above)

@gaearon
Copy link
Collaborator

gaearon commented Nov 5, 2017

#11463

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.

3 participants