-
Notifications
You must be signed in to change notification settings - Fork 74
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
Added detection for properties managed by enviromental variables #162
base: master
Are you sure you want to change the base?
Conversation
Hey @mtumiati. Thanks for the PR! Do you think we need to add any new tests for this? The current tests are passing, but I'm wondering if the expected results would differ when using |
@stovmascript I don't think that it's needed because if it is managed by the environmental variable it must be kept as it is. But tests are always good, so we can add one that cover this case. |
@stovmascript When will you merge it? |
I'd like to have a test for this before merging. |
I'd like to use
At this point, I don't know if I should ignore these environment variables, and let Right now, |
@Soreine Are you integrating React Native into an existing iOS project, or is it a fresh project bootstrapped with the React Native CLI? |
@stovmascript i'm running into the same issue as @Soreine. If you make your changes thru Xcode, it adds in the following variables
Would be nice if we could extend this PR so it detects both |
@alradadi we added a regex that detects any environmental variable with this shape $(VARIABLE) here is the function /**
* Determine if the given property is managed by an enviromental variable
* @param {String} text
* @return {Boolean} true if the given property is managed by an enviromental variable
*/
function isManagedByEnvVariable(propertyText) {
return propertyText ? !!/\$\(\S+\)/g.exec(propertyText) : false;
} here you can have a look at the regex https://regex101.com/r/yFCW4y/1 We just need to add some more tests and then merge. I hope to have time to do it very soon. |
It is a project that was bootstrapped with the React Native CLI. But I guess we made changes with XCode at some point and it added these variables. |
I create issue #186 before reading this pull request, It would be nice if |
thanks @archansel. We should detect if the version is managed by environmental variable first, and then change the MARKETING_VERSION and CURRENT_PROJECT_VERSION accordingly |
So how do you go about versioning with env vars in conjunction with RNV? Is it a separate process from when RNV does its versioning? Are those variables defined at build time? |
@stovmascript I checked #186 by @archansel. As he said these values are written in MyProject.xcodeproj/project.pbxproj. They should be easy to update |
I'll have to check what happens when you init a new RN app nowadays (and update our test fixtures) because AFAIK |
IMHO it will be better to manage both cases in order to have full compatibility |
I think it's not how RN init its project, its more about XCode itself. in XCode 11, when you change version number and build in the XCode UI, it will write the value to So as long as developer doesn't change the version or build manually in the XCode UI, current |
I wouldn't recommend doing things manually in Xcode - especially versioning, when already using RNV. Of course there will be things that need to be done, but I would then hope that developers would look through the diff of |
Just did a fresh I guess we could still merge this though because if developers implement env vars in plist files, you could bypass the whole step with plist parsing and updating. I wonder why RN doesn't do this in their plist templates by default... |
I could use this in our project - the project number getting reset back to 1 is a bit annoying with multiple targets. Any chance on this getting merged at some point? |
I finally added test to handle the case. @stovmascript you can check it out and decide to merge or not the pull request |
When this Pull request going to merge? |
It's a pity this hasn't been merged yet 😔 @iltumio are you able to rebase your fork with the latest from this repo so I can point my |
Nevermind, I've switched from using this package to using fastlane instead. Here's a good guide to setting it up: https://dev.to/osamaqarem/automatic-versioning-for-react-native-apps-2bf3 |
Any news? |
fixed issue #161
added a function that detects if a given property is managed by an enviromental variable like
in case CFBundleVersion property in Info.plist is managed that way, the value will not be replaced.