Closed elliotdickison closed 3 years ago
hey @elliotdickison
this is just to let you know I see your PR, but I'd need few more days to go over it.
thank you!
Thanks! Also just to reiterate, while the PR says +49,497 😱 my changes are actually fairly minimal (see the first commit - roughly +30 😅). The 10 billion lines of new code are all from the example test app bootstrapped with create-react-app (quarantined to the packages/example-web folder).
@maxkomarychev I need to implement config in my project in a couple weeks, do you think that timeframe is realistic for getting this PR merged/published? Just trying to plan in case I’ll need to come up with a different solution. Thanks!
I will review it by end of the week. Sorry for the delay.
Hey no worries - thanks for your work on this!
Will do! Yep, yaml will take web keys I’ll document that as well.
Updated the docs including
hi @elliotdickison I like final touches, however I see some tests are failing in your branch, can you please fix them?
btw, I also fixed github action so that when you push to the branch next time you will have test executed here in pr
thanks!
Fixed the tests. I forgot to re-run them after making this change:
// Only save xcconfig if the project contains an ios folder. All react-native
// apps will contain this folder, but some react-native-web apps may not.
if (fs.existsSync(path.join(project_root, "ios"))) {
The project_root in the render-env tests is fake so that condition was false and ios assets weren't being generated. I mocked existsSync in the render-env tests to return true so that ios assets are generated even though the ios folder doesn't technically exist.
I also squashed all the commits together.
@maxkomarychev Just FYI looks like the Github action won't run tests until after you approve - I ran them locally though and they are passing
@elliotdickison approved, let's see :)
thanks!
@maxkomarychev gitignore caught my test output file - updated the ignore rules should be good to go now
wow github should really let me approve all your further workflow runs 🤦
thanks @elliotdickison !!
It was surprisingly easy to get this working. I ended up generating a separate
index.web.js
file for the web and then using thebrowser
field in package.json to tell web bundlers to use this instead of the defaultindex.js
file. Thebrowser
field is official and supported by all popular bundlers. This is just a slight variation on what we discussed in #61, with the benefit that I didn't have to touch (and risk breaking) the existing native files. I also think it makes sense to keep the core react-native code in the main index.js entrypoint as this package is called react-native-ultimate-config after all.A lot of the code in this PR is just a default web react app generated with create-react-app to demonstrate the new web-compatible behavior (in a separate
example-web
lerna package). I split the addition of that example web app into a separate commit to make reviewing a little easier. I can drop a lot of the create-react-app boilerplate if you'd like. The only changes I made to the example-web package were: add react-native-ultimate-config as a dependency, add the rnuc npm script, and import/render a config value in App.tsx.I updated the spec tests to test the new index.web.js file output and then manually tested that types and config values work correctly when importing "react-native-web-config" into a web app (see
packages/example-web
).Fixes #61