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

remove console calls on production #8337

Closed
wants to merge 3 commits into from

Conversation

Emilios1995
Copy link
Contributor

@Emilios1995 Emilios1995 commented Jun 22, 2016

This adds to the production mode of babel-preset-react-native, a plugin to delete calls to the console while still preserving the side effects inside them.
The motivation is the great improvement on performance on the code running on device after removing the calls to console methods

Note that although the plugin works as expected, I don't know if this could cause any inconveniences on the rest of the system.

@ghost
Copy link

ghost commented Jun 22, 2016

By analyzing the blame information on this pull request, we identified @skevy and @frantic to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 22, 2016
@Emilios1995
Copy link
Contributor Author

CC @javache @bestander

@Emilios1995 Emilios1995 force-pushed the master branch 3 times, most recently from ac1e87a to 326053c Compare June 23, 2016 00:41
@Emilios1995
Copy link
Contributor Author

Notice: The commit was amended to simplify my original transform function.

@bestander
Copy link
Contributor

bestander commented Jun 23, 2016

Thanks, @Emilios1995, I'll post this internally at FB to ask teams if this affects them in any way and get back to you.
Don't be shy to nudge if I get distracted and forget to get back, but I'll try not to

@javache
Copy link
Member

javache commented Jun 23, 2016

We should keep console.error since that provides non-fatal error logging. You should also verify this doesn't break our console polyfill code and console error reporter code.

@javache
Copy link
Member

javache commented Jun 23, 2016

@Emilios1995
Copy link
Contributor Author

@javache I've changed the transform to ignore console.error
It's not identical, since mine preserves the side effects, as @ide suggested in #8285

const callee = path.get("callee")
if (callee.matchesPattern("console", true) && !callee.matchesPattern("console.error")) {
path.node.callee = template(`
(function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating a new function object each time, it might be simpler just to use Function.prototype, which is a no-op function #jstrivia

@Emilios1995
Copy link
Contributor Author

So, @bestander, do you think it's safe to merge?

@bestander
Copy link
Contributor

@Emilios1995, it seems that we use console.log in some products internally even for prod builds.
Any ideas how not to do it by default?
Can this plugin be in userland instead of baked into plugin?

@Emilios1995
Copy link
Contributor Author

Well, we could just suggest in the docs to include the official transform-remove-console in the production environment of the user's own .babelrc

@bestander
Copy link
Contributor

Yes, @Emilios1995, I think it is a good idea, want to send a docs PR?
Adding this setting by default may be confusing to people.

@bestander
Copy link
Contributor

Let's close this for now, feel free to reopen if there are more ideas

@bestander bestander closed this Jun 29, 2016
@stovmascript
Copy link

I tried using babel-plugin-transform-remove-console as specified in the performance docs in my RN project (v0.34.1) and also in a newly initialized AwesomeProject (v0.35.0), but it looks like the RN packager doesn't like the transformed code. Both throw the same error when running assembleRelease with gradle:

:app:bundleReleaseJsAndAssets
[2016-10-14 14:51:37] <START> Building Dependency Graph
[2016-10-14 14:51:37] <START> Crawling File System
[2016-10-14 14:51:37] <START> Finding dependencies
[2016-10-14 14:51:41] <END>   Crawling File System (3999ms)
[2016-10-14 14:51:41] <START> Building in-memory fs for JavaScript
[2016-10-14 14:51:41] <END>   Building in-memory fs for JavaScript (218ms)
[2016-10-14 14:51:41] <START> Building in-memory fs for Assets
[2016-10-14 14:51:41] <END>   Building in-memory fs for Assets (165ms)
[2016-10-14 14:51:41] <START> Building Haste Map
[2016-10-14 14:51:41] <START> Building (deprecated) Asset Map
[2016-10-14 14:51:41] <END>   Building (deprecated) Asset Map (59ms)
[2016-10-14 14:51:42] <END>   Building Haste Map (298ms)
[2016-10-14 14:51:42] <END>   Building Dependency Graph (4691ms)

TransformError: /Users/foo/AwesomeProject/node_modules/react-native/Libraries/JavaScriptAppEngine/Initialization/ExceptionsManager.js: /Users/foo/AwesomeProject/node_modules/react-native/Libraries/JavaScriptAppEngine/Initialization/ExceptionsManager.js: Property right of AssignmentExpression expected node to be of a type ["Expression"] but instead got null

:app:bundleReleaseJsAndAssets FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:bundleReleaseJsAndAssets'.
> Process 'command 'node'' finished with non-zero exit value 1

I also tested it on another project for the web, which is bundled with webpack and works as expected, so I didn't open an issue at the babel repo yet because it might be RN related.

facebook-github-bot pushed a commit that referenced this pull request Apr 26, 2017
Summary:
This PR adds a suggestion to the docs to use  the `transform-remove-console` babel plugin in production to remove `console.*` calls.
This information was previously in the docs, but was [removed](e759573) because the babel plugin [didn't work](#10412).
But now it's working well, as reported [here](#10412 (comment)), so it would be helpful to add the suggestion again.
Ideally, this would be done automatically, as I suggested in #8337
Closes #13651

Differential Revision: D4954872

Pulled By: hramos

fbshipit-source-id: 89ae1b813c50e678f0826f16ef88c8604e13d889
thotegowda pushed a commit to thotegowda/react-native that referenced this pull request May 7, 2017
Summary:
This PR adds a suggestion to the docs to use  the `transform-remove-console` babel plugin in production to remove `console.*` calls.
This information was previously in the docs, but was [removed](facebook@e759573) because the babel plugin [didn't work](facebook#10412).
But now it's working well, as reported [here](facebook#10412 (comment)), so it would be helpful to add the suggestion again.
Ideally, this would be done automatically, as I suggested in facebook#8337
Closes facebook#13651

Differential Revision: D4954872

Pulled By: hramos

fbshipit-source-id: 89ae1b813c50e678f0826f16ef88c8604e13d889
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants