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 #492

Merged
merged 1 commit into from
Sep 16, 2020
Merged

Add Windows support #492

merged 1 commit into from
Sep 16, 2020

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Sep 16, 2020

Add support for react-native-windows.

Update the docs and example and add an automatic CI test.

Add support for react-native-windows. Update the docs and example and
add an automatic CI test.
@height
Copy link

height bot commented Sep 16, 2020

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Copy link
Collaborator

@luancurti luancurti left a comment

Choose a reason for hiding this comment

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

Thanks!

@luancurti luancurti merged commit 8616212 into lugg:master Sep 16, 2020
@@ -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):
Copy link

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

Copy link
Contributor Author

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'
Copy link

Choose a reason for hiding this comment

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

space between include and < ?

Comment on lines +46 to +47
updateFile(nativeCode, path.join(outDir, 'RNCConfigValues.h'))
updateFile(rnCode, path.join(outDir, 'RNCConfigValuesModule.inc.h'))
Copy link

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`;
Copy link

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?

Copy link
Contributor Author

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

@hermanbanken
Copy link

@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.

https://github.com/hermanbanken/react-native-config/runs/5432726750?check_suite_focus=true

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.

None yet

4 participants