-
Notifications
You must be signed in to change notification settings - Fork 661
Add Windows support #492
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 #492
Conversation
Add support for react-native-windows. Update the docs and example and add an automatic CI test.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -32,7 +32,7 @@ Install the package: | |||
$ yarn add react-native-config | |||
``` | |||
|
|||
Link the library: | |||
Link the library (not supported on Windows, use manual linking): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we added autolinking to rnw on 63 (?) @jonthysell to confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did not work for me. I've opened an issue for that: microsoft/react-native-windows#6024
let nativeCode = ''; | ||
// React Native Module code | ||
let rnCode = ''; | ||
nativeCode += '#include<string>\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space between include
and <
?
updateFile(nativeCode, path.join(outDir, 'RNCConfigValues.h')) | ||
updateFile(rnCode, path.join(outDir, 'RNCConfigValuesModule.inc.h')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider naming these with .g to mark that they are autogenerated
const escaped = escapeString(value) | ||
nativeCode += ` inline static std::string ${key} = ${escaped};\n` | ||
rnCode += `REACT_CONSTANT(${key});\n` | ||
rnCode += `static inline const std::string ${key} = ${escaped};\n`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these could be made constexpr
instead and get rid of the nativeCode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on constexpr
I did two files to have a simpler namespace for the values in native code - i.e. ReactNativeConfig::something
instead of RNCConfig::ReactNativeConfigModule::something
@bzoz thanks for adding Windows support; unfortunately, the windows build seems to fail in GitHub Actions. Do you have any time to have a quick glance? I assume you have the most experience with the platform having contributed this (I have just a mac). I fixed one issue with msbuild, but there remains an error about having two similarly named solutions. |
Add support for react-native-windows.
Update the docs and example and add an automatic CI test.