Skip to content

Move DEV only module requires into __DEV__ blocks #10185

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Jul 19, 2017

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jul 14, 2017

This is related to #10176. Dangling require() calls were created from the build due to the usage not being in a __DEV__ block within the source code. This PR aims to move the require calls so they no longer dangle in the production builds.

Also, the need to have single quotes within modulesToStub blocks has been removed – it was error prone.

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

Nice, 👍

@gaearon
Copy link
Collaborator

gaearon commented Jul 19, 2017

Do you mind rebasing this?

@trueadm
Copy link
Contributor Author

trueadm commented Jul 19, 2017

@gaearon Done just now, please give it another check when you have time :) thanks!

@@ -24,7 +24,7 @@ var REACT_ELEMENT_TYPE =

if (__DEV__) {
var warning = require('fbjs/lib/warning');
var {getCurrentStackAddendum} = require('ReactComponentTreeHook');
var {getStackAddendum} = require('ReactDebugCurrentFrame');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like code change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops nevermind.

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.

OK

@gaearon
Copy link
Collaborator

gaearon commented Jul 19, 2017

Let's get this in. It's not a long term solution but at least it's consistent with what we already do.
Longer term we'll want #10176 (comment). We can look at it with ES modules transition.

@trueadm
Copy link
Contributor Author

trueadm commented Jul 19, 2017

Cool :) Thanks

@trueadm trueadm merged commit 357925a into facebook:master Jul 19, 2017
@gaearon gaearon deleted the dev-only-requires-properly-handled branch July 19, 2017 15:30
@abigsmall
Copy link
Contributor

abigsmall commented Jul 23, 2017

@trueadm I noticed that in some files there remain invocations of warning that aren’t explicitly wrapped in if (__DEV__) blocks, such as for getTouchIdentifier in ResponderTouchHistoryStore.js: is that something to be concerned about?

Sorry if commenting on already merged PRs is not recommended, figured I’d try this rather than create a new GitHub issue

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants