Adding option to opt-out of bundle caching and always contact the development server #1906

pre10der89 · 2018-07-17T23:55:51Z

If the JavaScript is changed between runs of the application the bundle loaded will not demonstrate the changes until Ctrl+R is executed. This breaks the previous developer cycle of being able to change the JavaScript and having those changes reflected on the next application run.

This was changed when the "HasUpToDateBundleInCache" check was added to the "ReactInstanceManager.CreateReactContextFromDevManagerAsync()" that will load the packager bundle from cache if it is up to date.

@rozele We discussed the possibility of adding a flag to allow the old behavior to be restored. This is what I've done in this PR. I've added a flag called "UseBundleCaching" to the ReactPage and ReactInstanceManager that will set the IsBundleCachingEnabled flag on the IDevSupportManager. If the flag is true and the cached bundle is up to date then the "CreateReactContextFromCachedPackagerBundleAsync" is executed; Otherwise the _devSupportManager.CreateReactContextFromPackagerAsync is executed.

I have chosen to set the default for the new flag to "true" so that the existing behavior is not changed. Let me know if you'd like the flag to be false by default, which will force anybody who wants to use the bundle caching to opt-in rather than the other way around.

…tructing the bundle when launching the application

pre10der89 · 2018-07-17T23:57:20Z

ReactWindows/Playground.Net46/AppReactPage.cs

+            }
+        }
+
+        public override bool UseBundleCaching


This demonstrates overriding the virtual UseBundleCaching flag. In this example, we opt out of bundle caching if building in a DEBUG configuration. Otherwise, bundle caching is enabled.

pre10der89 · 2018-07-18T00:00:00Z

ReactWindows/ReactNative.Net46/ReactPage.cs

@@ -97,6 +97,11 @@ public virtual Func<IJavaScriptExecutor> JavaScriptExecutorFactory
        /// </summary>
        public abstract bool UseDeveloperSupport { get; }

+        /// <summary>


Similar to how we pass the UseDeveloperSupport flag through the ReactPage I've added the "UseBundleCaching" flag on its way through the ReactInstanceManagerBuilder. I've set the default to "true" to keep the existing behavior. Please let me know whether we should change the default to "false" or if the default should be conditional.

pre10der89 · 2018-07-18T00:00:36Z

ReactWindows/ReactNative.Net46/ReactPage.cs

@@ -216,6 +221,7 @@ private ReactInstanceManager CreateReactInstanceManager()
            var builder = new ReactInstanceManagerBuilder
            {
                UseDeveloperSupport = UseDeveloperSupport,
+                UseBundleCaching = UseBundleCaching,


Passing the flag into the builder so that the ReactInstanceManager can set it on the DevSupportManager instance.

pre10der89 · 2018-07-18T00:02:22Z

ReactWindows/ReactNative.Shared/DevSupport/DevSupportManager.cs

@@ -110,6 +110,12 @@ public bool IsProgressDialogEnabled
            set;
        } = true;

+        public bool IsBundleCachingEnabled


DevSupportManager uses the flag in the form of "IsBundleCachingEnabled" to match the other options. I thought about adding this to the IDeveloperSettings property, but it didn't seem to fit there because that property is null in DisabledDevSupportManager. Doing so would have required checking for null in the "CreateReactContextFromDevManagerAsync.

hm. maybe DisabledDevSupportManager should be re-thought, then? instead of a Null Object Pattern, should it do something else.

I like the Null Object Pattern and think it's quite an effective mechanism for what the DevSupportManager does. Perhaps changing the DevInternalSettings to use the Null Object Pattern as well would alleviate the need for a null check.

I suppose a question at hand is how important is the IsBundleCachingEnabled flag? Is it important enough to be in the IDevSupportManager? How often will it need to be changed by different groups?

pre10der89 · 2018-07-18T00:03:04Z

ReactWindows/ReactNative.Shared/ReactInstanceManager.cs

@@ -513,7 +513,7 @@ private Task<ReactContext> CreateReactContextCoreAsync(CancellationToken token)

        private Task<ReactContext> CreateReactContextFromDevManagerAsync(CancellationToken token)
        {
-            if (_devSupportManager.HasUpToDateBundleInCache())
+            if (_devSupportManager.IsBundleCachingEnabled && _devSupportManager.HasUpToDateBundleInCache())


If caching is enabled and the bundle in cache is up to date then load from the cache... Otherwise create the bundle from the packager.

pre10der89 · 2018-07-18T00:04:17Z

ReactWindows/ReactNative.Shared/ReactInstanceManagerBuilder.cs

@@ -185,6 +198,13 @@ public ReactInstanceManager Build()
                _jsExecutorFactory,
                _nativeModuleCallExceptionHandler,
                _lazyViewManagersEnabled);
+
+            if (instance.DevSupportManager != null)


I didn't want to add another parameter to the ReactInstanceManager because that would break anybody who creates their own ReactInstanceManager. So, I set the flag after instantiation.

does null coalescing look any better here?

@matthargett

I originally had the following:

instance.DevSupportManager?.IsBundleCachingEnabled = _useBundleCaching;

but that produces a compiler error:

CS0131 The left-hand side of an assignment must be a variable, property or indexer

So I added the old-skool if test.

Technically, I think we'd be safe to have no null check here because the DevSupportManager is readonly and is always assigned to either an instance of DevSupportManager or DisabledDevSupportManager.

ok, let's eliminate the check then :)

matthargett · 2018-07-18T19:27:39Z

ReactWindows/ReactNative.Shared/DevSupport/DevSupportManager.cs

@@ -110,6 +110,12 @@ public bool IsProgressDialogEnabled
            set;
        } = true;

+        public bool IsBundleCachingEnabled


hm. maybe DisabledDevSupportManager should be re-thought, then? instead of a Null Object Pattern, should it do something else.

matthargett · 2018-07-18T20:49:33Z

ReactWindows/ReactNative.Shared/ReactInstanceManagerBuilder.cs

@@ -185,6 +198,13 @@ public ReactInstanceManager Build()
                _jsExecutorFactory,
                _nativeModuleCallExceptionHandler,
                _lazyViewManagersEnabled);
+
+            if (instance.DevSupportManager != null)


does null coalescing look any better here?

rozele · 2018-07-19T01:18:25Z

My main hesitation with this PR is that it's inconsistent with the behavior on other platforms. I think most react-native developers coming from other platforms would be accustomed to the bundle caching behavior.

pre10der89 · 2018-07-19T01:38:24Z

@rozele I'm all for bundle caching, but as we discussed last week the expected developer experience is no longer working. Stopping an application, then making changes to the JavaScript, and then running the application again shouldn't require that the developer then have to hit Ctrl+R to get the bundle (with the new JavaScript) to be reloaded.

As a developer I'd expect my JavaScript changes to be reflected the next time I ran the application when using the dev server.

I would have expected the HasUpToDateBundleInCache would return false in this case. Could it be that the HasUpToDateBundleInCache is broken?

reseul · 2018-07-19T01:50:21Z

@pre10der89 I think HasUpToDateBundleInCache checks if the bundle is up to date with the native app (so bundle is not older than the native app), not the other way around.
I understand your complaint, cause I got confused by this initially as well.
I'm not sure how easy is to make that method better, the bundler would have to offer some support as well,

rozele · 2018-07-19T02:18:32Z

+1 to @reseul's comment - I think a bundler feature for this could be very helpful for all platforms.

pre10der89 · 2018-07-19T02:19:59Z

@reseul I believe you are correct and I may have had things backward. Prior to the addition of the cache bundle code the packager was always being told to reconstruct the bundle. This allowed changes made to the JavaScript to be included in a new application run. This is what you'd expect to happen if any code in your application changes and you "rebuild" and run. If you change your Xaml code and ran the application again the change would be realized.

Without the ability to force the packager to reconstruct the bundle in development mode we essentially will always have to load the native modules twice; once when the application starts and once when Ctrl+R is manually invoked by the developer to reload the new JavaScript. Since one of our native modules is a media stack this could be expensive. Not do mention the poor developer experience of having to hit Ctrl+R and the inevitable frustration when they forget to hit Ctrl+R and cannot figure out why their changes are not being reflected (this happened to me).

I am not familiar with how the Android and iOS development works. I'm guessing that people start the simulator and leave it running all day and just hit Ctrl+R when they make changes. That reloads the bundle but doesn't technically restart the application.

I believe that desktop application development is slightly different. Often times you run the application, evaluate your changes, and then shut it down. Sure, you could do Ctrl+R with Windows as well, but since our application is quite complex we've found that our reloads do not always work perfectly. Whether it is our fault or not.

My proposal in this PR is to provide an optional way to toggle the caching option in any scenario applicable to the application. My personal opinion is that all non-bundle debug builds should be defaulted to tell the packager to reconstruct the bundle. That would allow us a better developer experience for the Windows platform and would avoid confusion from all that follow me. But, I understand that that "default" position might not be the view of the repository. I do believe that the developer should have the option to always rebundle.

reseul · 2018-07-19T02:33:51Z

@pre10der89 I do know that somehow you DON'T have to refresh when developing on iOS. Not sure how it works, maybe that "Enable Hot Reloading" is set there to true... Which I think we also have in RNW (never tried it). It may do what you want!

pre10der89 · 2018-07-19T03:03:32Z

@reseul "Live Reload" will cause an automatic Ctrl+R when the packager detects a change to one of the JavaScript files. You'll see the "Loading" dialog in this case. "Hot Reloading" seems to reload the JavaScript or at least the view without seeing the "Loading Dialog". Hot Reloading has always been flaky.

Neither of these options will cause a re-bundle on restarting the application.

The DevInternalSettings seem to be mostly associated with the Ctrl+D menu. We could put the proposed flag in these settings but it wouldn't make since to have it in the Ctrl+D menu since it is an OnCreate decision.

I guess we need to look at how iOS is able to both cache the bundle and reconstruct the bundle when the application is restarted.

reseul · 2018-07-19T03:22:06Z

@pre10der89 Yes, you're right, you want this to happen even when not in "dev mode".

pre10der89 · 2018-07-27T05:39:05Z

@rozele @reseul I confirmed your findings that iOS does not have this problem. It will contact the dev server on every application launch. I also confirmed that Android has the same behavior as Windows. Both Windows and Android require that a reload be executed, which simply contacts the development server anyway.

The WPF implementation of HasUpToDateCachedBundle() has some errors that I have fixed in PR#1915

Those changes, however, still do not address my issue with the caching mechanism. The workflow of an Android and iOS application is different from the workflow of a Windows desktop application. As are the workflow of applications with different complexities. My team nearly always restarts the application for any number of reasons. This means we are always affected by the cached bundle.

With my changes from PR#1915 I've mapped out all relevant permutations of native and JavaScript changes. In all cases, the application was stopped and restarted.

Case 1: Neither Native nor Bundle Exists ----> Bundle is Created ---> OK
Case 2: No Changes ----> Cached Bundled Loaded ---> OK
Case 3: JavaScript Changes Only ----> Cached Bundled Loaded ---> Not Expected -- The cache is now out of sync with the source.
Case 4: Native Changes Only ----> Bundle is rebuilt ---> OK, since we don't know whether a React Native Module interface has changed.
Case 5: No Changes ----> Cached Bundled Loaded ---> OK

The only place where the bundle is updated is when the native code changes. When the JavaScript changes nothing happens and now the cached bundle is out of sync with the source code. This is equivalent to not updating an EXE when native code changes. In order to realize the JavaScript changes I have to reload, which contacts the dev server and rebuilds the bundle.

I am not sure what benefit the cached bundle has beyond the case where nothing has changed and I want to run the application multiple times. It has no benefit when development on the JavaScript side is in progress. Even with live reloading this is still a problem whenever the application is restarted. A developer will have to remember they can only make changes in live reloading or they must reload after the application has started. This seems strange to me...

This PR is asking that we simply add an opt-out for the caching functionality so that the old style of development can be restored if desired. This is similar to opting-out from dev support with the useDeveloperSupport . I'm certainly open to renaming or reworking how the opt-out is code. Either call it IsBundleCachingEnabled or "AlwaysCreateReactContextFromPackager"

ghost · 2020-04-03T21:01:22Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

Adding a flag to disable bundle caching to restore support for recons…

8fac281

…tructing the bundle when launching the application

pre10der89 commented Jul 17, 2018

View reviewed changes

pre10der89 commented Jul 18, 2018

View reviewed changes

Restoring use of using System to fix ReleaseBundle build

f19bae8

matthargett suggested changes Jul 18, 2018

View reviewed changes

rozele mentioned this pull request Jul 23, 2018

Incorrect use of cached bundle when dev server is no longer running #1784

Closed

pre10der89 changed the title ~~Adding a flag to disable bundle caching to restore the ability to load changes on application launch~~ Adding option to opt-out of bundle caching and always contact the development server Jul 27, 2018

rozele mentioned this pull request Jan 3, 2019

Changes to App.windows.js not reflected (app was working fine previously but now only shows default placeholder view) #2019

Closed

chrisglein added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Mar 27, 2020

ghost added the no-recent-activity Issue/PR has gone stale and may be closed (label applied by bot) label Apr 3, 2020

ghost closed this Apr 10, 2020

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding option to opt-out of bundle caching and always contact the development server #1906

Adding option to opt-out of bundle caching and always contact the development server #1906

pre10der89 commented Jul 17, 2018

pre10der89 Jul 17, 2018

pre10der89 Jul 18, 2018

pre10der89 Jul 18, 2018

pre10der89 Jul 18, 2018

matthargett Jul 18, 2018

pre10der89 Jul 18, 2018

pre10der89 Jul 18, 2018

pre10der89 Jul 18, 2018

matthargett Jul 18, 2018

pre10der89 Jul 18, 2018

matthargett Aug 16, 2018

matthargett Jul 18, 2018

matthargett Jul 18, 2018

rozele commented Jul 19, 2018

pre10der89 commented Jul 19, 2018

reseul commented Jul 19, 2018

rozele commented Jul 19, 2018

pre10der89 commented Jul 19, 2018

reseul commented Jul 19, 2018

pre10der89 commented Jul 19, 2018

reseul commented Jul 19, 2018

pre10der89 commented Jul 27, 2018 •

edited

Loading

ghost commented Apr 3, 2020

Adding option to opt-out of bundle caching and always contact the development server #1906

Adding option to opt-out of bundle caching and always contact the development server #1906

Conversation

pre10der89 commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rozele commented Jul 19, 2018

pre10der89 commented Jul 19, 2018

reseul commented Jul 19, 2018

rozele commented Jul 19, 2018

pre10der89 commented Jul 19, 2018

reseul commented Jul 19, 2018

pre10der89 commented Jul 19, 2018

reseul commented Jul 19, 2018

pre10der89 commented Jul 27, 2018 • edited Loading

ghost commented Apr 3, 2020

pre10der89 commented Jul 27, 2018 •

edited

Loading