Adds a dummy function to assist react devtools to look for minification #9661

ManasJayanth · 2017-05-11T07:33:19Z

Patch for react-devtools#694. Refer comment

gaearon · 2017-05-11T10:29:50Z

src/renderers/dom/fiber/ReactDOMFiber.js

+  // Refer https://github.com/facebook/react-devtools/issues/694#issuecomment-300535376
+  var dummyMinificationTestFn = function() {
+    if (__DEV__) {
+      var anotherDummyNoop = function() {};


How do we detect whether this code block was eliminated, judging by the source?
It can be tricky because it could be, for example, minified but not eliminated.

I think we should instead do something like return 42; in __DEV__ block instead.
Then, on DevTools side, we can check both that:

The function didn’t return anything (DEV mode check)

The function doesn’t contain 42 literal (dead code elimination check)

Fair point. Making the changes.

gaearon · 2017-05-11T10:30:01Z

src/renderers/dom/fiber/ReactDOMFiber.js

@@ -559,9 +559,19 @@ var ReactDOM = {
 };

 if (typeof injectInternals === 'function') {
+  // A dummy function to check for minification during runtime
+  // Refer https://github.com/facebook/react-devtools/issues/694#issuecomment-300535376
+  var dummyMinificationTestFn = function() {


Let's rename it to testMinification.

gaearon · 2017-05-11T10:32:13Z

We should also probably refactor this a bit and put the testMinification injection into injectInternals itself. Since it's shared between all renderers and isn't specific to ReactDOM.

While you do that, could you please also remove conditional exports from ReactFiberDevToolsHook? This won't work with ES6 transition anyway. Instead we should always export them, but put their contents into a condition (which should only be evaluated when the module is initialized).

ManasJayanth · 2017-05-11T10:55:20Z

While you do that, could you please also remove conditional exports from ReactFiberDevToolsHook? This won't work with ES6 transition anyway. Instead we should always export them, but put their contents into a condition (which should only be evaluated when the module is initialized).

I understand the concern, but I couldn't find any conditional exports in ReactFiberDevToolsHook

gaearon · 2017-05-11T11:00:32Z

Sorry I wasn’t clear. I mean stuff like this:

let injectInternals = null;
let onCommitRoot = null;
let onCommitUnmount = null;

Let's just keep them functions that are always properly exported, but change body of the functions depending on whether we found the hook or not.

ManasJayanth · 2017-05-11T11:03:44Z

Also,

What about warning if one is not using devtools? Should there be a 'not a production build' warning too like how the stack ReactDOM warns currently?
Inject testMinification in injectInternals like this?

inject(
  Object.assign( {}, internals, { testMinification })
);

Or use object-assign. I vaguely remember some discussion about avoiding Object.assign inside React.

gaearon · 2017-05-11T12:15:51Z

What about warning if one is not using devtools? Should there be a 'not a production build' warning too like how the stack ReactDOM warns currently?

Let's do whatever ReactDOM is doing for Fiber too.

Inject testMinification in injectInternals like this?

Yes, or we could make those functions explicit positional arguments.

I vaguely remember some discussion about avoiding Object.assign inside React.

It's fine to use it, as we compile it to use a polyfill.

ManasJayanth · 2017-05-11T13:38:11Z

@gaearon Yet to fix conditional exports. If these commits look good, I'll push in that fix as well.

gaearon · 2017-05-11T13:40:09Z

I would prefer that we get rid of internals and just pass those two functions as regular arguments.

ManasJayanth · 2017-05-12T05:09:24Z

I would prefer that we get rid of internals and just pass those two functions as regular arguments.

I'm confused. Can you clarify what two functions you are talking about? Also pass them as positional params to what? inject? But inject only accepts renderer as an object.

ManasJayanth · 2017-05-12T13:12:02Z

@gaearon ^^

gaearon · 2017-05-12T15:52:13Z

I mean instead of passing injectInternals({funcA, funcB}) and then Object.assign-ing another function there, we can do injectInternals(funcA, funcB) and then create the target object once.

ManasJayanth · 2017-05-12T16:13:15Z

Oh. That will affect renderers trying to use injectInternals. Eg. RN itself uses it. Are you sure? If yes, update RN renderer too?

gaearon · 2017-05-12T16:17:27Z

Yes, please update them both.

ManasJayanth · 2017-05-15T14:30:31Z

@gaearon Pushed the changes requested. Pending: conditional exports issue.

gaearon · 2017-05-15T16:06:07Z

src/renderers/shared/fiber/ReactFiberDevToolsHook.js

+  injectInternals = function(
+    findFiberByHostInstance: (instance: any) => Fiber,
+    findHostInstanceByFiber: (component: Fiber) => any,
+    bundleType: number,


I think bundleType or version can be calculated here instead of being passed as arguments.

ManasJayanth · 2017-05-16T14:53:32Z

@gaearon Pushed the requested changes

ManasJayanth · 2017-05-24T19:05:29Z

@gaearon ReactNativeInspector just got added yesterday or so. So as we were discussing earlier, pass it along in the args and make it optional, given that ReactDOMFiber is not sending anything similar?

gaearon · 2017-05-25T16:23:29Z

Hey, sorry for the churn. There's many competing things happening at the same time and it gets a bit tricky to synchronize them. I guess let's go back to your previous approach with Object.assign since we're still adding more stuff there.

ManasJayanth · 2017-05-26T14:44:38Z

@gaearon I can understand :) I have pushed the changes. Please have a look. Again, conditional exports is yet to be dealt with.

gaearon · 2017-05-26T14:54:45Z

src/renderers/shared/fiber/ReactFiberDevToolsHook.js

+        'the production build which skips development warnings and is faster. ' +
+        'See https://fb.me/react-minification for more details.',
+    );
+    rendererID = rendererID = inject(


Unintentional double assignment?

Sorry. Don't know how I missed that.

ManasJayanth · 2017-05-29T14:26:06Z

@gaearon Good to go?

ManasJayanth · 2017-06-15T13:31:21Z

@gaearon Does this PR need more work?

gaearon · 2017-06-15T16:43:50Z

src/renderers/shared/fiber/ReactFiberDevToolsHook.js

+        return 42;
+      }
+    };
+    warning(


injectInternals only runs if DevTools exists.
Could you please move this out of the method?

@gaearon Move testMinification out of injectInternals? Do we need it if there DevTools are not installed?

We still should show the warning even if DevTools don't exist.

…tion

ManasJayanth · 2017-08-21T06:11:18Z

@gaearon Hi. Quite some time has passed by since this PR was raised. Is the idea behind it still valid? Should I resolve the conflicts and continue work?

gaearon · 2017-09-13T20:06:22Z

Sorry. I went on a holiday and later we’ve been in crunch mode fixing remaining issues in 16.

We did something related in #10446 but then reverted in #10673 due to the discussion in #10640.

I’m going to take another final look at this now and see if this approach still makes sense. We need some way to detect badly envified bundle but I’m not sure this is enough (or does the right thing) now.

ManasJayanth · 2017-09-14T13:59:42Z

@gaearon No problem :)

So if I understand right, calling toString() to check for minification is not an option anymore for reasons explained in #10640 comment (What an explanation btw! So much going on behind the scenes)

So is checking for minification still a valid problem to solve? I'm refering to this comment here

Why do we allow this to not be minimized at all?
The other concern was that if shipping works because of this bail out. Then if byte code reversing gets deployed, then it'll skip this and start failing. Without the site doing anything differently.

Is Sebastian trying to say that we are tacking the problem at the wrong place? Or that the build process itself must ensure minification (else face byte code reversing issues) and beyond the build process there's nothing much we can do.

ManasJayanth · 2017-09-14T14:02:24Z

Does it also mean there's no spec compliant way to retrieve the source string?

gaearon · 2017-09-14T14:17:35Z

So if I understand right, calling toString() to check for minification is not an option anymore for reasons explained in #10640 comment

Not exactly—we can call it but shouldn’t do so from inside of react-dom for every single consumer. I settled on this approach as an alternative: #10702 and facebook/react-devtools#888. As you can see it’s not affected by those problems because:

We only call toString for people who have React DevTools which is better than doing this for everyone
We call it from inside of React DevTools so if it becomes problematic we can immediately disable it

I’m sorry I didn’t go ahead with your PR—unfortunately I didn’t realize the approach was flawed, and had to spend some time myself yesterday to figure out how to proceed.

And I also didn’t realize your PR doesn’t solve the problem with flat bundles. If the user has bad dead code elimination they will still get a minified function inside of React. That’s why putting the check outside was important.

ManasJayanth · 2017-09-14T14:38:52Z

Ah, It alright. Better luck next time I guess. Thanks nevertheless.

Oh, and I was wondering, is it annoying as repo maintainers if contributors ping your on Twitter to get an update/feedback on a PR that's getting stale? Whats the right code of conduct in such a case? There are times when wished repo maintainers were available on IRC or something to give their opinion on an approach. Something more real time.

gaearon · 2017-09-14T14:47:48Z

You can always ping me in Twitter DM if you submitted a PR, just make it clear in first message :-)

In general we try to be more responsive. It's just been very hard to keep track of things as we have the giant checklist related to the new implementation of everything. I think we'll pick up some steam after 16 and be able to sort through contributions more quickly.

ManasJayanth · 2017-09-14T15:24:01Z

I can understand :) Great to hear this!

facebook-github-bot added the CLA Signed label May 11, 2017

gaearon requested changes May 11, 2017

View reviewed changes

ManasJayanth force-pushed the inject-minification-testfn branch from dbd61db to cce7f65 Compare May 11, 2017 13:02

ManasJayanth force-pushed the inject-minification-testfn branch from cce7f65 to 15a2daa Compare May 13, 2017 06:32

gaearon reviewed May 15, 2017

View reviewed changes

ManasJayanth force-pushed the inject-minification-testfn branch from 15a2daa to a7cbb97 Compare May 15, 2017 16:22

ManasJayanth force-pushed the inject-minification-testfn branch from a7cbb97 to 0535f53 Compare May 26, 2017 14:15

gaearon reviewed May 26, 2017

View reviewed changes

ManasJayanth force-pushed the inject-minification-testfn branch from 0535f53 to 4626242 Compare May 26, 2017 15:09

ManasJayanth mentioned this pull request May 11, 2017

Fiber dev mode detection using a test function facebook/react-devtools#709

Closed

ManasJayanth force-pushed the inject-minification-testfn branch from 4626242 to dcea5ed Compare June 13, 2017 14:44

gaearon reviewed Jun 15, 2017

View reviewed changes

ManasJayanth force-pushed the inject-minification-testfn branch from dcea5ed to 65354d4 Compare June 30, 2017 04:31

ManasJayanth added 2 commits June 30, 2017 10:19

Adds testMinification to assist react devtools to look for minifica…

4ae60df

…tion

Adds a warning for minified development build

70aa887

ManasJayanth force-pushed the inject-minification-testfn branch from 65354d4 to 70aa887 Compare June 30, 2017 04:54

gaearon closed this Sep 14, 2017

ManasJayanth deleted the inject-minification-testfn branch September 14, 2017 15:22

Adds a dummy function to assist react devtools to look for minification #9661

Adds a dummy function to assist react devtools to look for minification #9661

Conversation

ManasJayanth commented May 11, 2017

gaearon May 11, 2017 • edited Loading

Choose a reason for hiding this comment

ManasJayanth May 11, 2017

Choose a reason for hiding this comment

gaearon May 11, 2017

Choose a reason for hiding this comment

gaearon commented May 11, 2017

ManasJayanth commented May 11, 2017

gaearon commented May 11, 2017 • edited Loading

ManasJayanth commented May 11, 2017 • edited Loading

gaearon commented May 11, 2017 • edited Loading

ManasJayanth commented May 11, 2017

gaearon commented May 11, 2017

ManasJayanth commented May 12, 2017

ManasJayanth commented May 12, 2017

gaearon commented May 12, 2017

ManasJayanth commented May 12, 2017

gaearon commented May 12, 2017

ManasJayanth commented May 15, 2017

gaearon May 15, 2017

Choose a reason for hiding this comment

ManasJayanth commented May 16, 2017

ManasJayanth commented May 24, 2017

gaearon commented May 25, 2017

ManasJayanth commented May 26, 2017

gaearon May 26, 2017

Choose a reason for hiding this comment

ManasJayanth May 26, 2017

Choose a reason for hiding this comment

ManasJayanth commented May 29, 2017

ManasJayanth commented Jun 15, 2017

gaearon Jun 15, 2017

Choose a reason for hiding this comment

ManasJayanth Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon Jun 29, 2017

Choose a reason for hiding this comment

ManasJayanth commented Aug 21, 2017 • edited Loading

gaearon commented Sep 13, 2017

ManasJayanth commented Sep 14, 2017 • edited Loading

ManasJayanth commented Sep 14, 2017

gaearon commented Sep 14, 2017 • edited Loading

ManasJayanth commented Sep 14, 2017

gaearon commented Sep 14, 2017

ManasJayanth commented Sep 14, 2017 • edited Loading

gaearon May 11, 2017 •

edited

Loading

gaearon commented May 11, 2017 •

edited

Loading

ManasJayanth commented May 11, 2017 •

edited

Loading

gaearon commented May 11, 2017 •

edited

Loading

ManasJayanth Jun 16, 2017 •

edited

Loading

ManasJayanth commented Aug 21, 2017 •

edited

Loading

ManasJayanth commented Sep 14, 2017 •

edited

Loading

gaearon commented Sep 14, 2017 •

edited

Loading

ManasJayanth commented Sep 14, 2017 •

edited

Loading