Add an 'Experiments' class to enable/disable experiments.

[satya164]

To enable an experiment (say HMR),

Experiments.enableExperiment(Experiments.HOT_MODULE_REPLACEMENT)

To check if an experiment is enabled,

if (Experiments.isExperimentEnabled(Experiments.HOT_MODULE_REPLACEMENT) {
  // do stuff
}

See #5339

Test plan - In a fresh project "Enable Hot Module Replacement" shouldn't exist in dev menu. It should appear after enabling HMR in MainActivity's onCreate.

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @none to be a potential reviewer.

[ide]

A couple of thoughts -- is there a way to scope experiments to the ReactInstanceManager object? Ex: this allows for separate ReactInstanceManagers to run different experiments. Also how about using strings or something more self-descriptive than ints for the experiment IDs so that if we persist them on disk there isn't the risk of someone deleting an experiment with ID = 0, and then later adding a new experiment with ID = 0 and confusing the two?

[satya164]

Thanks for the inputs @ide

is there a way to scope experiments to the ReactInstanceManager object? Ex: this allows for separate ReactInstanceManagers to run different experiments.

Maybe we can use namespaces? Right now we don't really have many experiments. But if we have in future, the enableExperiment method can take 2 parameters maybe? Need to think about it.

Also how about using strings or something more self-descriptive than ints for the experiment IDs so that if we persist them on disk there isn't the risk of someone deleting an experiment with ID = 0, and then later adding a new experiment with ID = 0 and confusing the two?

Yeah. makes sense. I'll change it to use strings :)

[facebook-github-bot]

@satya164 updated the pull request.

[mkonicek]

I see you've landed another PR that adds a menu option for HMR. What's the reason for this PR?

[satya164]

@mkonicek This PR hides the menu item by default as HMR is not ready yet. People who want to test it can enable it in their MainActivity and the menu item will appear.

See #5339 for context on this.

[mkonicek]

Add Javadoc explaining what the reason this class exist is please.

[satya164]

Will add the docs in a bit.

[mkonicek]

Cool, thanks for the explanation! I just noticed we show the menu item on master but enabling it doesn't do anything.

[satya164]

@mkonicek Enabling it should add hot=true to the bundle request URL (default is hot=false).

[facebook-github-bot]

@satya164 updated the pull request.

[satya164]

@mkonicek Done.

[mkonicek]

nit: typo in exprimental

small nit: Could say "exposes a way for app developers to enable/disable..." or similar - to make it clear this is public API intended to be called by the apps, not just by the framework.

[mkonicek]

Some small nits and let's shipit :)

[satya164]

Wow! That's a ton of typos! Fixed :)

[facebook-github-bot]

@satya164 updated the pull request.

[facebook-github-bot]

@satya164 updated the pull request.

[mkonicek]

👍

@facebook-github-bot shipit

[mkonicek]

Good idea with the strings @ide!

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/215773062097347/int_phab to review.

[mkonicek]

Import failed because of an internal Buck file.

[mkonicek]

The Buck file was actually useful at highlighting that this way we have a circular dependency between java/com/facebook/react and java/com/facebook/react/devsupport.

Would it be cleaner to have a method on ReactInstanceManager like James suggested, e.g. setExperiments(Experiments) and in MainActivity, do:

@Override
protected Experiments getExperiments() {
    return new Experiments("hot_module_replacement");
}

Also realized we might have a problem with introducing the Experiments.HOT_MODULE_REPLACEMENT constant - now people will use that in their code and once we'll want to "promote" HMR from an experiment to a real feature we'll want to remove that constant from the Experiments class. But we can't because that would break existing apps. Therefore the string in the example above.

Also, does the hot module reloading work on Android in 0.19.0-rc if you enable the experiment and enable it in the menu? I tried it by changing the render function in my index.android.js and it didn't work.

We're introducing the Experiments public API purely for this single feature (HMR). Should we wait until we have more use cases like this before we introduce a public API we won't be able to easily change later without breaking apps?

How about the following?

Ship HMR as a menu option for everyone once it works well on Android. For now, remove the options.put in DevSupportManagerImpl.

[satya164]

Would it be cleaner to have a method on ReactInstanceManager like James suggested, e.g. setExperiments(Experiments) and in MainActivity

That was my initial idea actually. But then I thought that it's probably easier not to tie it to ReactInstanceManager. Because then everyone who wants to check if the experiment is enabled will need access to the instance of ReactInstanceManager.

Also realized we might have a problem with introducing the Experiments.HOT_MODULE_REPLACEMENT constant - now people will use that in their code and once we'll want to "promote" HMR from an experiment to a real feature we'll want to remove that constant from the Experiments class. But we can't because that would break existing apps. Therefore the string in the example above.

As per my thinking, the Experiments class is meant to break frequently. That's why I named it Experiments. People who use it are supposed to know that it's used only for testing. But as you said, someone might promote it and people might start using it. In that case we have to make it clear that it's only meant for testing and will likely to break.

One way to do it will be to change the method name to Experiments.ThisWillBreakIKnowWhatIamDoing.enableExperiment() or something similar to make it clear that it will break. Stealing this idea from React web which has a dangerouslySetInnerHTML prop.

Also, does the hot module reloading work on Android in 0.19.0-rc if you enable the experiment and enable it in the menu? I tried it by changing the render function in my index.android.js and it didn't work.

No it doesn't work yet. But @martinbigio told he had made some progress on Android front also :)

We're introducing the Experiments public API purely for this single feature (HMR). Should we wait until we have more use cases like this before we introduce a public API we won't be able to easily change later without breaking apps?

My thinking was that we could use it for enabling more experimental features in future, which are work in a progress and needs testing from the community. The main idea behind this new class is for people who want to test new experimental features can test them easily. Changing the source code of React to enable a feature is troublesome for testing, because we'll be installing from master a lot as we're testing, and it'll be overwritten everytime. That's why I wanted a way to enable it in the app's code.

The Buck file was actually useful at highlighting that this way we have a circular dependency between java/com/facebook/react and java/com/facebook/react/devsupport.

It prolly makes more sense to put this class under devsupport.

Thanks for all the points. I understand the problems arising due to this. If you wanna remove it, I'm fine with it. We can think of a better way of doing this in the future. :D

[mkonicek]

Because then everyone who wants to check if the experiment is enabled will need access to the instance of ReactInstanceManager.

Yup, making it static is easier but introduces those circular dependencies between packages which are usually a code smell.

As per my thinking, the Experiments class is meant to break frequently.

Right, but anyone who has enabled some experiments in the past will see their code break every time we remove an experiment. We could get around this by not using those Java constants but just strings.

No it doesn't work yet.

If it doesn't work at all in 0.19, why let people enable it via Experiments only to realize it doesn't work?

It probably makes more sense to put this class under devsupport.

Experiments will be related to other parts of the code than devsupport though.

Thanks for all the points. I understand the problems arising due to this. If you wanna remove it, I'm fine with it. We can think of a better way of doing this in the future. :D

No problem! I should have thought about it more in details when you messaged me first but I'm sometimes a bit overwhelmed, sorry!

[satya164]

Yup, making it static is easier but introduces those circular dependencies between packages which are usually a code smell.

Experiments will be related to other parts of the code than devsupport though.

Yeah. I understand. We need to give this some thought, on how to enable features for testing purposes only in future.

No problem! I should have thought about it more in details when you messaged me first but I'm sometimes a bit overwhelmed, sorry!

No issues. I should also have thought about this a bit more.

@mkonicek should I send a PR commenting out the menu item?

[mkonicek]

Yes please let's comment out the menu item for now. Thank you and thanks for understanding!