Added React Native Windows dotNet46 support #684

abodalevsky · 2017-01-31T18:53:35Z

Added support react-native-windows dotNet
Added example for react-native-windows UWP based
Added example for react-native-windows dotNet based

Project structure:

CodePush.Shared - shared code between UWP and dotNet
CodePush - UWP specific code
CodePush.Net46 - dotNet specific code

For UWP solution it needs to be added the following projects:

CodePush.Shared
CodePush

For dotNet solution it needs to be added the following projects:

CodePush.Shared
CodePush.Net46

Examples:

Examples\CodePushDemoApp\windows\CodePushDemoApp.sln the solution contains both examples (UWP and dotNet).

Notes

Example for ARM configuration has not been tested. Since there is no changes in UWP part of implementation, there is low risk of failure.

In this implementation we tried to reuse UWP library as much as possible. The following issues are relevant for both platforms:

ZipFile.ExtractToDirectory is not reliable and throws exception if:
- folder exists already
- path is too long (> 250 chars)
Un-zipping is quite long operation. Does it make sense for async?
- await UpdateUtils.UnzipBundleAsync(downloadFile.Path, unzippedFolder.Path);

…hould return null

msftclas · 2017-01-31T18:53:38Z

Hi @abodalevsky, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

rozele · 2017-02-06T17:06:25Z

windows/CodePush.Shared/CodePushNativeModule.cs

        internal async Task LoadBundleAsync()
        {
            // #1) Get the private ReactInstanceManager, which is what includes
            //     the logic to reload the current React context.
            FieldInfo info = typeof(ReactPage)
                .GetField("_reactInstanceManager", BindingFlags.NonPublic | BindingFlags.Instance);
-
+#if WINDOWS_UWP
            var reactInstanceManager = (ReactInstanceManager)typeof(ReactPage)
                .GetField("_reactInstanceManager", BindingFlags.NonPublic | BindingFlags.Instance)
                .GetValue(_codePush.MainPage);


This is more of a question for the CodePush team, but should we just make the fields you need here part of the public API on react-native-windows?

@rozele That would be ideal and would help us to avoid being broken by changes, but it's up to you if that's your priority. We do the same kind of reflection on the other platforms, although we would like to at some point in the distant future submit PR's so that we can get rid of it.

rozele · 2017-02-06T17:07:30Z

windows/CodePushReactPackage.cs


 namespace CodePush.ReactNative
 {
    public sealed class CodePushReactPackage : IReactPackage
    {
        private static CodePushReactPackage CurrentInstance;
-        private static bool NeedToReportRollback = false;
-


This is suspicious, is this some missing feature, or was it just totally dead code?

It was part of @geof90 's original commit (according to git blame). The value is set to true, but never read from. Looks like it was transliterated in to C# incorrectly, maybe? We can file and issue and do a separate PR to fix -- we specifically have user stories in our project for update failure modes that will likely force us to fill missing gaps on that front.

@matthargett I suspect the purpose of this is to post to the CodePush server after a rollback happens, so that users can see a record of the number of rollbacks in the CodePush CLI. If you'd like to file an issue and do a separate PR I am fine with that!

rozele · 2017-02-06T17:08:47Z

windows/CodePush.Shared/SettingsManager.cs

        private static ApplicationDataContainer Settings = ApplicationData.Current.LocalSettings.CreateContainer(CodePushConstants.CodePushPreferences, ApplicationDataCreateDisposition.Always);
+#else
+        private static ApplicationDataContainer Settings = new ApplicationDataContainer();


new ApplicationDataContainer(); [](start = 59, length = 31)

Are these settings in-memory only then? I think ApplicationData.Current.LocalSettings is persisted across runs.

You are right absolutely! Thanks for this catch.
Fixed in 03af9fc

abodalevsky · 2017-02-07T15:48:13Z

Regarding PCLStorage. I use it for Net46 project only. In fact this doesn't add additional dependency because it is already exists in ReactNative.Net46 project.

In general, probably, it make sense to use PCLStorage for UWP implementation as well. A benefit will be more code shared between UWP & NET implementations. As I mentioned in comments above I tried not to modify existent implementation UWP, as this is not the subject of this PR.

richardhuaaa · 2017-02-09T18:39:03Z

.gitignore

@@ -150,3 +150,37 @@ captures/

 # Remove after this framework is published on NPM
 code-push-plugin-testing-framework/node_modules
+


Can you mirror these changes to the .npmignore file, and if possible, could you also leave a comment at the top of this file informing future contributors that they need to do the same?

I'm also assuming there's nothing you want to exclude from the npm package that isn't already listed here, but please add them there if so!

@Silhouettes Updated. (Alex is offline for a few hours)

Thank you @kevinvangelder! I also added UnitTest project to .npmignore

richardhuaaa · 2017-02-09T18:51:26Z

@rozele Do you think your and @matthargett's reviews are sufficient to merge this?

rozele · 2017-02-09T22:17:50Z

@Silhouettes I'll take another pass and then approve.

rozele · 2017-02-09T22:20:58Z

package.json

-    "run-sequence": "latest"
+    "run-sequence": "latest",
+    "tslint": "^4.3.1",
+    "typescript": "^2.1.5"


Any specific reason why these were added?

Nothing special, just wanted to avoid npm UNMET PEER DEPENDENCY warnings:

npm WARN gulp-tslint@7.1.0 requires a peer of tslint@>=4.0.0-dev but none was installed.
npm WARN tslint@4.4.2 requires a peer of typescript@>=2.0.0 but none was installed

rozele · 2017-02-09T22:22:54Z

windows/CodePush/UpdateManager.cs

+					it needs to be handled			 
+				 */
+                throw e;
+            }


This will screw up the stack trace of the exception (the point where the exception is thrown is recalculated from here). Since this has no net affect on the behavior other, please remove this catch block.

rozele · 2017-02-09T22:25:04Z

windows/CodePush.Net46/UpdateManager.cs

+            {
+                throw e;    //left here for debug purposes to see the reason of ZipFile failure.
+            }
+


Another case of a catch block with no purpose.

rozele · 2017-02-09T22:29:52Z

@rihua@microsoft.com one architectural comment about the shared code project. If we were shipping code push as a compiled binary, it wouldn't matter so much, but now users have to add two projects to their react-native-windows solutions to see all code from the code push module. Obviously the shared code approach saves a bunch of duplicate code for WPF. The only other alternative would be to request that a copy of all code be made instead of using a shared project. Just thought I should point this out, as it could impact perceived debuggability for code push users.

In reply to: 278792526 [](ancestors = 278792526)

rozele · 2017-02-09T22:30:28Z

Other than the final comments I've made, and the general comment about shared projects, this looks good.

In reply to: 278795588 [](ancestors = 278795588,278792526)

matthargett · 2017-02-10T02:59:29Z

@rozele we can follow up with a separate PR that creates a NuGet package for CodePush, if you like. Since CodePush isn't like React Native Windows where people may want to extend the core source project with new files, it should probably be a NuGet package regardless of this PR for the best DX.

richardhuaaa · 2017-02-11T00:29:51Z

I will merge this PR on Monday unless anyone still wants to make changes.

Thanks everyone for the great work and the very informative comments :)

maxim-pop · 2017-03-02T22:45:22Z

Hi @abodalevsky ,
The Demo App contains a .pfx certificate file (/CodePushDemoApp/windows/CodePushDemoApp/CodePushDemoApp_TemporaryKey.pfx).
Is it possible to remove it from repo? We have compliance issues with storing secrets in code.

abodalevsky · 2017-03-06T10:50:20Z

Hi @maxim-pop

VisualStudio has generated this certificate by default within the project. In general it should be used for signing the assembly for deployment purposes. But I don't see in code that it is used. Therefore I may assume that for this example the *_TemporaryKey.pfx may be safely removed.

I have created PR #741, please take a look.

maxim-pop · 2017-03-06T22:18:00Z

Thanks!

Alexander Bodalevsky added 29 commits January 18, 2017 13:57

Added missed dev dependencies

1523905

Added Windows example

01863ff

Switched to the latest RNW

c861f45

Shared between UWP & NET code is encapsulated into separate project.

765fa86

Added implementation WindowsStorage abstraction

c003b28

Adapted mostly all abstractions to Net46

c8a4eb4

Fixed: If no data in ApplicationDataContainer it throws exception - s…

25eaa8b

…hould return null

Added WPF example

af2bfa3

WPF: Added download new version

d40fe61

App version is returned as SemVer

e064e6f

Some clean up code

41bd279

Allign with fix in react-native-windows (PR#950)

11a363d

CodePushUtils moved to shared project

0d19899

Added Release/Debug configurations for example

b2a300b

Fixed path to NuGet packages

628219b

Fixed path to NuGet packages, second attempt

5a85171

Added x64 configuration

81dd65f

Removed unneccesary exception handler

c24ce0c

NET46: Load ChakraCore.dll via NuGet myget

cfbb85a

Clean code

2558abc

Merged upstream

30511ff

Fixed code formating

3608fa8

Clean up the code

fc6de48

Changed Net46 app name.

07cde44

clean up the code

217a762

Ignored windows autogenerated files

a8cfe04

Added necessary for ARM configuration images

e43a4a8

Fixed review notes

950e126

Added reference to ReactNative project

d9c2028

rozele reviewed Feb 6, 2017

View reviewed changes

Alexander Bodalevsky added 2 commits February 7, 2017 13:50

fixes after review

13e8f4c

NET46: Added persistance to ApplicationDataContainer class

03af9fc

Alexander Bodalevsky added 4 commits February 8, 2017 14:25

ChakraCore preview package updated to Microsoft myget feed

a5083c5

removed unnecessary dependency

85a2546

moved to react-native-windows 0.40.0-rc.1

935ace1

Fixed: NET46 application can not create AppStorage in some cases.

e07526a

richardhuaaa reviewed Feb 9, 2017

View reviewed changes

update gitignore and npmignore

8091f73

rozele reviewed Feb 9, 2017

View reviewed changes

cleanup stacktrace-hijacking catch blocks

a3e8ab3

Exclude tests from NPM

2ff806f

Merge branch 'master' into feature/net46

e529016

richardhuaaa merged commit be96f07 into microsoft:master Feb 14, 2017

abodalevsky deleted the feature/net46 branch March 6, 2017 08:47

piyush965 mentioned this pull request Jun 5, 2018

React native code push supported on WPF platform but lack of support documentation. #1304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added React Native Windows dotNet46 support #684

Added React Native Windows dotNet46 support #684

abodalevsky commented Jan 31, 2017 •

edited

Loading

msftclas commented Jan 31, 2017

rozele Feb 6, 2017

richardhuaaa Feb 9, 2017

rozele Feb 6, 2017

matthargett Feb 8, 2017

richardhuaaa Feb 9, 2017

rozele Feb 6, 2017

abodalevsky Feb 7, 2017

abodalevsky commented Feb 7, 2017

richardhuaaa Feb 9, 2017

kevinvangelder Feb 9, 2017

abodalevsky Feb 10, 2017

richardhuaaa commented Feb 9, 2017

rozele commented Feb 9, 2017

rozele Feb 9, 2017

abodalevsky Feb 10, 2017

rozele Feb 9, 2017

rozele Feb 9, 2017

rozele commented Feb 9, 2017

rozele commented Feb 9, 2017

matthargett commented Feb 10, 2017

richardhuaaa commented Feb 11, 2017

maxim-pop commented Mar 2, 2017

abodalevsky commented Mar 6, 2017

maxim-pop commented Mar 6, 2017

		@@ -150,3 +150,37 @@ captures/

		# Remove after this framework is published on NPM
		code-push-plugin-testing-framework/node_modules

Added React Native Windows dotNet46 support #684

Added React Native Windows dotNet46 support #684

Conversation

abodalevsky commented Jan 31, 2017 • edited Loading

Project structure:

Examples:

Notes

msftclas commented Jan 31, 2017

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

abodalevsky commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardhuaaa commented Feb 9, 2017

rozele commented Feb 9, 2017

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 Feb 9, 2017

rozele commented Feb 9, 2017

matthargett commented Feb 10, 2017

richardhuaaa commented Feb 11, 2017

maxim-pop commented Mar 2, 2017

abodalevsky commented Mar 6, 2017

maxim-pop commented Mar 6, 2017

abodalevsky commented Jan 31, 2017 •

edited

Loading