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

Report bad dead code elimination to React DevTools #10702

Merged
merged 2 commits into from Sep 13, 2017

Conversation

Projects
None yet
4 participants
@gaearon
Member

gaearon commented Sep 13, 2017

Fixes #9589 (again).
We started with #10446, then reverted it in #10673 over concerns in #10640.

This time we don’t rely on toString inside ReactDOM itself. Instead we report to React DevTools if they exist, passing the function itself as an argument.

React DevTools can check for ^_^ there and both produce the “red React” and potentially even do the setTimeout trick to report the error to analytics.

This doesn’t have the same concerns as explained in #10640 because we’re not doing it for every user but only for React developers who visit React-powered websites which are also built with CommonJS. So it’s a smaller slice. If we suddenly can’t rely on toString anymore we can always cut that code from DevTools.

@gaearon gaearon requested a review from sebmarkbage Sep 13, 2017

@gaearon gaearon added this to the 16.0 milestone Sep 13, 2017

@gaearon gaearon requested review from sophiebits and bvaughn Sep 13, 2017

@sophiebits

Seems OK to me but I'll let @sebmarkbage review. Would the string ever get hoisted and changed to look up from a string table and therefore not be in the source?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 13, 2017

Member

It it does get hoisted that would be a false negative (no warning when we should warn) so doesn’t seem very dangerous. Worst case, we just remove this check.

Member

gaearon commented Sep 13, 2017

It it does get hoisted that would be a false negative (no warning when we should warn) so doesn’t seem very dangerous. Worst case, we just remove this check.

@sebmarkbage

Neat. I would prefer this to check something already in the package so we don't need to add this particular piece of scope.

// Don't change the message. React DevTools relies on it. Also make sure
// this message doesn't occur elsewhere in this function, or it will cause
// a false positive.
throw new Error('^_^');

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 13, 2017

Member

Wasting bytes.

@sebmarkbage

sebmarkbage Sep 13, 2017

Member

Wasting bytes.

This comment has been minimized.

@gaearon

gaearon Sep 13, 2017

Member

Shorter suggestions?

@gaearon

gaearon Sep 13, 2017

Member

Shorter suggestions?

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 13, 2017

Member

throw 0;? :)
or
throw null;

@sebmarkbage

sebmarkbage Sep 13, 2017

Member

throw 0;? :)
or
throw null;

This comment has been minimized.

@gaearon

gaearon Sep 14, 2017

Member

I wanted something that would be hard to accidentally introduce into the function elsewhere. Because then you'd get a false positive. I guess I could search for throw instead?

@gaearon

gaearon Sep 14, 2017

Member

I wanted something that would be hard to accidentally introduce into the function elsewhere. Because then you'd get a false positive. I guess I could search for throw instead?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Sep 13, 2017

Member

piece of scope?

Member

sophiebits commented Sep 13, 2017

piece of scope?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 13, 2017

Member

The problem with putting it inside of the bundle is process.env.NODE_ENV would get processed.. by us (during Rollup build). So I’m not sure how to accomplish that without doing something very convoluted. Putting it here also clearly signals this is CommonJS-specific.

Member

gaearon commented Sep 13, 2017

The problem with putting it inside of the bundle is process.env.NODE_ENV would get processed.. by us (during Rollup build). So I’m not sure how to accomplish that without doing something very convoluted. Putting it here also clearly signals this is CommonJS-specific.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Sep 13, 2017

Member
function checkDCE() {
  if (__DEV__) {
    throw;
  }
  DEVTOOLS.check(checkDCE);
}

if (!__DEV__) {
  checkDCE();
}
Member

sophiebits commented Sep 13, 2017

function checkDCE() {
  if (__DEV__) {
    throw;
  }
  DEVTOOLS.check(checkDCE);
}

if (!__DEV__) {
  checkDCE();
}
@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Sep 13, 2017

Member

As a bonus, that would help ensure we don't mess up our UMD builds. :)

Member

sophiebits commented Sep 13, 2017

As a bonus, that would help ensure we don't mess up our UMD builds. :)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 13, 2017

Member

Hmm.. Maybe that works! It’s a bit hard for me to think about...

Member

gaearon commented Sep 13, 2017

Hmm.. Maybe that works! It’s a bit hard for me to think about...

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 13, 2017

Member

I don’t think that would work.
It would become this in the .cjs.production.min.js:

function checkDCE() {
  DEVTOOLS.check(checkDCE);
}

checkDCE();

and would seem successful.

But then the user might not apply DCE in their code, and so they’ll actually ship both .cjs.development.js (which will not run but still exist) and .cjs.production.min.js.

Am I wrong?

Member

gaearon commented Sep 13, 2017

I don’t think that would work.
It would become this in the .cjs.production.min.js:

function checkDCE() {
  DEVTOOLS.check(checkDCE);
}

checkDCE();

and would seem successful.

But then the user might not apply DCE in their code, and so they’ll actually ship both .cjs.development.js (which will not run but still exist) and .cjs.production.min.js.

Am I wrong?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon
Member

gaearon commented Sep 13, 2017

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 13, 2017

Member

I’ll get this in as is for now but I’m happy to iterate if there are specific proposals for improvements.

(The one @sophiebits suggested wouldn’t work because we DCE CJS bundles ourselves to avoid process.env.NODE_ENV access hit in Node.)

Member

gaearon commented Sep 13, 2017

I’ll get this in as is for now but I’m happy to iterate if there are specific proposals for improvements.

(The one @sophiebits suggested wouldn’t work because we DCE CJS bundles ourselves to avoid process.env.NODE_ENV access hit in Node.)

@gaearon gaearon merged commit aebd7f5 into facebook:master Sep 13, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@gaearon gaearon deleted the gaearon:report-bad-dce branch Sep 13, 2017

@bvaughn bvaughn referenced this pull request Sep 14, 2017

Closed

React 16 RC #10294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment