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

Report bad dead code elimination to React DevTools #10702

Merged
merged 2 commits into from
Sep 13, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon 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.

Copy link
Contributor

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

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('^_^');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wasting bytes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shorter suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

throw 0;? :)
or
throw null;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

piece of scope?

@gaearon
Copy link
Collaborator Author

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
Copy link
Contributor

sophiebits commented Sep 13, 2017

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

if (!__DEV__) {
  checkDCE();
}

@sophiebits
Copy link
Contributor

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

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 13, 2017

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

@gaearon
Copy link
Collaborator Author

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
Copy link
Collaborator Author

gaearon commented Sep 13, 2017

DevTools PR: facebook/react-devtools#888

@gaearon
Copy link
Collaborator Author

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

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.

4 participants