Skip to content

[act] Wrap IsThisRendererActing in DEV check #16259

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

Merged
merged 2 commits into from
Jul 31, 2019

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jul 31, 2019

So that it doesn't leak into the production bundle. Follow-up to #16240.

So that it doesn't leak into the production bundle. Follow-up to facebook#16240.
@sizebot
Copy link

sizebot commented Jul 31, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 31, 2019

@threepointone Just to confirm, this isn't supposed to affect prod, right? There are some tests that are failing (correction: one test) but I assume that was just an oversight in the last PR.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 31, 2019

I'm going to assume that this doesn't affect prod and we can follow up later if that's not the case. All the act stuff should be dev-only, or alternatively, in a separate testing build.

@threepointone
Copy link
Contributor

We use this sigil not just for warnings now, but also for deciding when to flush, so it does actually affect prod?

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 31, 2019

Is this the only case? We really should not be leaking act behavior into the main production bundles.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 31, 2019

What is the use case for running act tests in production?

@threepointone
Copy link
Contributor

I think the discussion we had was people might be running tests with prod builds? iirc. Happy to not support it if so, or move to making testing builds.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 31, 2019

Since this only affects Concurrent and Sync mode I'll merge this and we can pick up discussion elsewhere.

@acdlite acdlite merged commit f939df4 into facebook:master Jul 31, 2019
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.

5 participants