Skip to content
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

Add Windows support #530

Merged
merged 6 commits into from
Oct 23, 2020
Merged

Add Windows support #530

merged 6 commits into from
Oct 23, 2020

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Oct 12, 2020

Summary

Add support for Windows using react-native-windows.

This PR includes the necessary Windows files and changes to the docs.

Test Plan

Unfortunately, this implementation uses APIs not available in Windows build uses by Github Action. You can run tests manually, using the example app - using Appium and jest:

yarn appium
yarn test:windows

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I mentioned this change in CHANGELOG.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)

Add support for react-native-windows.
@zoontek
Copy link
Owner

zoontek commented Oct 13, 2020

@bzoz Hi @bzoz and thanks, this is an incredible pull request!
I read the files and it seems really clean to me. I really would like to test it before merging it (and release it with the next major version - #522), but need to find a Windows device for it since I don't have any 😕

windows/README.md Outdated Show resolved Hide resolved
windows/README.md Outdated Show resolved Hide resolved
@bzoz
Copy link
Contributor Author

bzoz commented Oct 13, 2020

@zoontek you can use Windows evaluation VMs to test: https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/:

When setting up, make sure to up the CPU count and RAM for the VM.

In the VM, run Powershell as admin, and execute:

Set-ExecutionPolicy Unrestricted -Scope Process -Force; iex (New-Object System.Net.WebClient).DownloadString('https://raw.githubusercontent.com/microsoft/react-native-windows/master/vnext/Scripts/rnw-dependencies.ps1')

Answer y to all questions, it will setup tools needed on Windows, etc.

Then run choco install git.install --params "/GitAndUnixToolsOnPath" --force, to get rm in the PATH as it is needed by the example install script.

Then, its just building and running the example, in another console (so it will refresh PATH variable):

git clone https://github.com/zoontek/react-native-permissions.git
cd react-native-permissions
git fetch origin pull/530/head:win
git checkout win
yarn
cd example
yarn
npx react-native run-windows

The last one will take some time to build stuff. The script will try to install the app on the VM, and it can timeout. If that happens, just rerun npx react-native run-windows - it will be fast, and you should see the app running. Some of the deps do not support Windows, so you wont get nice icons, but the app should work:
image

Co-authored-by: Alexander Sklar <[email protected]>
@@ -59,7 +61,8 @@
},
"peerDependencies": {
"react": ">=16.8.6",
"react-native": ">=0.60.0"
"react-native": ">=0.60.0",
"react-native-windows": ">=0.62"
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be in the module always? I forget where we landed on this @jonthysell @acoates-ms

@asklar
Copy link
Contributor

asklar commented Oct 13, 2020

@zoontek you can use Windows evaluation VMs to test: https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/:

When setting up, make sure to up the CPU count and RAM for the VM.

In the VM, run Powershell as admin, and execute:

Set-ExecutionPolicy Unrestricted -Scope Process -Force; iex (New-Object System.Net.WebClient).DownloadString('https://raw.githubusercontent.com/microsoft/react-native-windows/master/vnext/Scripts/rnw-dependencies.ps1')

Answer y to all questions, it will setup tools needed on Windows, etc.

Then run choco install git.install --params "/GitAndUnixToolsOnPath" --force, to get rm in the PATH as it is needed by the example install script.

Then, its just building and running the example, in another console (so it will refresh PATH variable):

git clone https://github.com/zoontek/react-native-permissions.git
cd react-native-permissions
git fetch origin pull/530/head:win
git checkout win
yarn
cd example
yarn
npx react-native run-windows

The last one will take some time to build stuff. The script will try to install the app on the VM, and it can timeout. If that happens, just rerun npx react-native run-windows - it will be fast, and you should see the app running. Some of the deps do not support Windows, so you wont get nice icons, but the app should work:
image

@bzoz @zoontek you can also use Windows Sandbox instead of setting up the VM (though you'll still have to install the dev dependencies)
image

@@ -0,0 +1,163 @@
<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" ToolsVersion="16.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$(SolutionDir)\packages\Microsoft.Windows.CppWinRT.2.0.200316.3\build\native\Microsoft.Windows.CppWinRT.props" Condition="Exists('$(SolutionDir)\packages\Microsoft.Windows.CppWinRT.2.0.200316.3\build\native\Microsoft.Windows.CppWinRT.props')" />
Copy link

@rectified95 rectified95 Oct 13, 2020

Choose a reason for hiding this comment

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

I think this should be: $(SolutionDir)packages -> no slash after the env variable. Have you also tried consuming the module in a new app, other than the existing example in this repo? (Unless we have changed/fixed the trailing slash)
Ref:
react-native-checkbox/react-native-checkbox@e94ba09

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

@rectified95 rectified95 left a comment

Choose a reason for hiding this comment

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

Awesome! Just make sure you can consume this from a clean app before merging.

@zoontek
Copy link
Owner

zoontek commented Oct 16, 2020

Unfortunately, I'm only have my work computer with multiple iOS and Android SDKs installed and not enough space for this 😕

Screenshot 2020-10-16 at 14 25 04

I have an old NUC (Gigabyte BRIX w. Celeron N2807) without hard drive. I will buy a small SSD.
Performances will be horrible, but it should be enough to test and maintain it. 😅

src/module.windows.ts Outdated Show resolved Hide resolved
src/module.windows.ts Outdated Show resolved Hide resolved
src/constants.ts Outdated Show resolved Hide resolved
@bzoz
Copy link
Contributor Author

bzoz commented Oct 20, 2020

@zoontek updated, PTAL

@zoontek zoontek merged commit d969b46 into zoontek:master Oct 23, 2020
@zoontek
Copy link
Owner

zoontek commented Oct 23, 2020

@bzoz Tested again, everything seems OK now. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants