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

Added detection for properties managed by enviromental variables #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iltumio
Copy link

@iltumio iltumio commented Nov 20, 2019

fixed issue #161

added a function that detects if a given property is managed by an enviromental variable like

$(CURRENT_PROJECT_VERSION)

in case CFBundleVersion property in Info.plist is managed that way, the value will not be replaced.

@stovmascript
Copy link
Owner

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 $(CURRENT_PROJECT_VERSION).

@iltumio
Copy link
Author

iltumio commented Nov 20, 2019

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

@love8587
Copy link

love8587 commented Feb 1, 2020

@stovmascript When will you merge it?

@stovmascript
Copy link
Owner

I'd like to have a test for this before merging.

@Soreine
Copy link

Soreine commented Mar 5, 2020

I'd like to use react-native-version but I noticed the project I created defines MARKETING_VERSION and CURRENT_PROJECT_VERSION, and uses MARKETING_VERSION in Info.plist:

	<key>CFBundleShortVersionString</key>
	<string>$(MARKETING_VERSION)</string>

CURRENT_PROJECT_VERSION is not used though.

At this point, I don't know if I should ignore these environment variables, and let react-native-version fill hard-coded values, or if I should keep these variables and patch react-native-version to update CURRENT_PROJECT_VERSION and MARKETING_VERSION instead.

Right now, react-native-version seems to hard-code values, and update CURRENT_PROJECT_VERSION, but not MARKETING_VERSION, so it's not clear what's the expected behavior.

@stovmascript
Copy link
Owner

@Soreine Are you integrating React Native into an existing iOS project, or is it a fresh project bootstrapped with the React Native CLI?

@alradadi
Copy link

alradadi commented Mar 6, 2020

@stovmascript i'm running into the same issue as @Soreine. If you make your changes thru Xcode, it adds in the following variables

<key>CFBundleShortVersionString</key>
<string>$(MARKETING_VERSION)</string>
<key>CFBundleVersion</key>
<string>$(CURRENT_PROJECT_VERSION)</string>

Would be nice if we could extend this PR so it detects both MARKETING_VERSION and CURRENT_PROJECT_VERSION

@iltumio
Copy link
Author

iltumio commented Mar 6, 2020

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

@Soreine
Copy link

Soreine commented Mar 10, 2020

@Soreine Are you integrating React Native into an existing iOS project, or is it a fresh project bootstrapped with the React Native CLI?

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.

@archansel
Copy link

archansel commented Jun 14, 2020

I create issue #186 before reading this pull request, It would be nice if react-native-version change env vars instead of hardcoding the value (only if using env vars). Since XCode 11, it will use MARKETING_VERSION and CURRENT_PROJECT_VERSION if you change it manually in XCode

@iltumio
Copy link
Author

iltumio commented Jun 15, 2020

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

@stovmascript
Copy link
Owner

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?

@iltumio
Copy link
Author

iltumio commented Jun 15, 2020

@stovmascript I checked #186 by @archansel. As he said these values are written in MyProject.xcodeproj/project.pbxproj. They should be easy to update

@stovmascript
Copy link
Owner

I'll have to check what happens when you init a new RN app nowadays (and update our test fixtures) because AFAIK $(CURRENT_PROJECT_VERSION) was always used in plists. Now that we also have $(MARKETING_VERSION) it could mean we might drop updating plists altogether.

@iltumio
Copy link
Author

iltumio commented Jun 15, 2020

IMHO it will be better to manage both cases in order to have full compatibility

@archansel
Copy link

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 MyApp.xcodeproj > project.pbxproj under buildSettings and overwrite Info.plist using $(MARKETING_VERSION) and $(CURRENT_PROJECT_VERSION)

So as long as developer doesn't change the version or build manually in the XCode UI, current react-native-version implementation works just fine (at least that's what I do)

@stovmascript
Copy link
Owner

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 .pbxproj and other files changed by Xcode, see what has changed and reverted unnecessary changes in order for tools like RNV to function properly.

@stovmascript
Copy link
Owner

Just did a fresh npx react-native init AwesomeProject and it looks like the stock plist files have hardcoded values for both CFBundleShortVersionString and CFBundleVersion.

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

@zsiegel
Copy link

zsiegel commented Jul 29, 2020

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?

@iltumio
Copy link
Author

iltumio commented Oct 17, 2020

I finally added test to handle the case. @stovmascript you can check it out and decide to merge or not the pull request

@chetanparmar95
Copy link

When this Pull request going to merge?

@se1exin
Copy link

se1exin commented Apr 22, 2021

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 package.json to your repo with the updates?

@se1exin
Copy link

se1exin commented Apr 26, 2021

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 package.json to your repo with the updates?

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

@Darex1991
Copy link

Any news?

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

10 participants