Skip to content

Take applicationId from package.json #848

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 10, 2018
Merged

Take applicationId from package.json #848

merged 4 commits into from
May 10, 2018

Conversation

KristianDD
Copy link
Contributor

Take applicationId from package.json

Description

While still overwrite of applicationId from app.gradle is possible, user is able to specify a platform specific identifier inside package.json. This addresses problems related to livesync not working when applicationId is changed inside app.gradle

Does your commit message include the wording below to reference a specific issue in this repo?

Fixes NativeScript/nativescript-cli#3040.

Related Pull Requests

NativeScript/nativescript-cli#3120

Does your pull request have [unit tests]

@KristianDD KristianDD mentioned this pull request Sep 13, 2017
Merged
@NativeScript NativeScript deleted a comment from ns-bot Sep 14, 2017
petekanev
petekanev suggested changes Sep 15, 2017
View reviewed changes
build-artifacts/project-template-gradle/build.gradle Outdated
@@ -724,18 +736,20 @@ copyTypings.onlyIf({

task validateAppIdMatch {
Copy link
Contributor

Choose a reason for hiding this comment

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

This task appears to be almost identical to setAppIdentifier. You can either extract the common logic in a static function used by both tasks. Or think whether setAppIdentifier should be so early in the configuration phase, because at that point another gradle configuration may overwrite the id.

I'd consider calling setAppIdentifier's code implicitly only on debug builds (meaning put it in a debug-build check) - replacing the app identifier automatically with what's in the package.json, informing the user of the implicit actions, while leaving the application identifier untouched otherwise, if it has already been set someplace else in Gradle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will extract the 4 common lines of the tasks in a static function.

The behavior you describe is almost the total opposite of what we've decided on the meeting for this feature and what has been approved in the meeting notes from you, @rosen-vladimirov, @PanayotCankov, @Plamen5kov and other teem members.

We set application identifier that early so that we keep our current behavior and don't force the configuration on the user. If the configuration from package.json is overwritten in later configuration stages the user will get a fair warning that this is not the recommended way and that Livesync will not work properly.

Having the app identifier set one way in debug and another way (and from another place) in release build sounds strange to me. I'd never expect a tooling to behave this way.

Plamen5kov
Plamen5kov reviewed Sep 25, 2017
View reviewed changes
build-artifacts/project-template-gradle/build.gradle Outdated
def appIdentifier = "";
if(packageJsonMap){
appIdentifier = packageJsonMap.nativescript.id;
if (!(appIdentifier instanceof String))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does CLI check if appIdentifier object is valid? if not please do some kind of a check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes there is validation for the id property inside CLI:
https://github.com/NativeScript/nativescript-cli/blob/master/lib/services/android-project-service.ts#L474
https://github.com/NativeScript/nativescript-cli/blob/master/lib/services/android-project-service.ts#L95
https://github.com/NativeScript/nativescript-cli/blob/master/lib/commands/run.ts#L40

Plamen5kov
Plamen5kov suggested changes Sep 25, 2017
View reviewed changes
Copy link
Contributor

@Plamen5kov Plamen5kov left a comment

Choose a reason for hiding this comment

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

please, look at the comments.

@KristianDD KristianDD force-pushed the kddimitrov/application-id-taken-from-packagejson branch from 94d98cb to fb06ea6 Compare October 24, 2017 15:35
@Plamen5kov
Copy link
Contributor

Plamen5kov commented Oct 25, 2017 •
edited
Loading

@KristianDD you need to rebase to master for the CI to run

@KristianDD KristianDD force-pushed the kddimitrov/application-id-taken-from-packagejson branch from fb06ea6 to 4e95aa6 Compare November 8, 2017 15:34
@ns-bot
Copy link

ns-bot commented Nov 8, 2017

💚

@NativeScript NativeScript deleted a comment from ns-bot Nov 13, 2017
@NativeScript NativeScript deleted a comment from ns-bot Nov 13, 2017
@NativeScript NativeScript deleted a comment from ns-bot Nov 13, 2017
@NativeScript NativeScript deleted a comment from ns-bot Nov 13, 2017
@NativeScript NativeScript deleted a comment from ns-bot Nov 13, 2017
@NativeScript NativeScript deleted a comment from KristianDD Nov 13, 2017
@NativeScript NativeScript deleted a comment from ns-bot Nov 13, 2017
@NativeScript NativeScript deleted a comment from ns-bot Nov 13, 2017
@NativeScript NativeScript deleted a comment from ns-bot Nov 13, 2017
@KristianDD KristianDD force-pushed the kddimitrov/application-id-taken-from-packagejson branch from 4e95aa6 to d62ef74 Compare December 20, 2017 18:28
@ns-bot
Copy link

ns-bot commented Dec 20, 2017

💚

@KristianDD
Copy link
Contributor Author

Task validateAppIdMatch doesn't seem to be executed, because the if statement here is not satisfied. It seems to work with:
if (currentTask =~ /assembleDebug/ || currentTask =~ /assembleRelease/) {.
@Plamen5kov what do you think? Should I change the if statement?

@Plamen5kov
Copy link
Contributor

@KristianDD I've changed the if statement to be backward compatible, so it should match both assembleRelease/Debug which is the old behavior and assembleFlavor1Flavor2...Release/Debug, I'll check out what you mean and get back to you. Until then, let's leave it as it is for now if it's not urgent.

@petekanev
Copy link
Contributor

@KristianDD please don't change the if-statement for the time being, currently it can pick up all flavors of assemble, while assembleDebug/Release will only conform to these two.

@KristianDD KristianDD force-pushed the kddimitrov/application-id-taken-from-packagejson branch from d62ef74 to ccbed06 Compare January 15, 2018 13:07
@NativeScript NativeScript deleted a comment from ns-bot Jan 22, 2018
@NativeScript NativeScript deleted a comment from ns-bot Jan 22, 2018
@KristianDD
Copy link
Contributor Author

run ci

@ns-bot
Copy link

ns-bot commented Jan 22, 2018

💚

@ns-bot
Copy link

ns-bot commented Jan 25, 2018

💚

@ns-bot
Copy link

ns-bot commented Jan 29, 2018

💚

petekanev
petekanev reviewed Jan 30, 2018
View reviewed changes
build-artifacts/project-template-gradle/app/build.gradle Outdated
@@ -56,6 +56,48 @@ def computeCompileSdkVersion = { -> project.hasProperty("compileSdk") ? compileS
def computeTargetSdkVersion = { -> project.hasProperty("targetSdk") ? targetSdk : 26 }
def computeBuildToolsVersion = { -> project.hasProperty("buildToolsVersion") ? buildToolsVersion : "26.0.1" }

def getAppIdentifier = { packageJsonMap ->
def appIdentifier = "";
if(packageJsonMap){
Copy link
Contributor

Choose a reason for hiding this comment

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

indents please

petekanev
petekanev reviewed Jan 30, 2018
View reviewed changes
build-artifacts/project-template-gradle/app/build.gradle Outdated
}

def getProjectName = { ->
if(project.hasProperty("nsApplicationIdentifier")){
Copy link
Contributor

Choose a reason for hiding this comment

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

indent + format indentation

petekanev
petekanev reviewed Jan 30, 2018
View reviewed changes
build-artifacts/project-template-gradle/app/build.gradle Outdated
println "\t + setting applicationId";
File packageJsonFile = new File("$rootDir/../../package.json");

if(packageJsonFile.exists())
Copy link
Contributor

Choose a reason for hiding this comment

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

indent

petekanev
petekanev reviewed Jan 30, 2018
View reviewed changes
build-artifacts/project-template-gradle/app/build.gradle Outdated
def packageJsonMap = jsonSlurper.parseText(content);
def appIdentifier = getAppIdentifier(packageJsonMap);

if(appIdentifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

indent

petekanev
petekanev reviewed Jan 30, 2018
View reviewed changes
build-artifacts/project-template-gradle/app/build.gradle Outdated
def lineSeparator = System.getProperty("line.separator")

if (project.hasProperty("nsApplicationIdentifier") && !project.hasProperty("release")) {
if(project.nsApplicationIdentifier != android.defaultConfig.applicationId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

indent

petekanev
petekanev reviewed Jan 30, 2018
View reviewed changes
build-artifacts/project-template-gradle/app/build.gradle Outdated
def errorMessage = "${lineSeparator}WARNING: The Application identifier is different from the one inside \"package.json\" file.$lineSeparator" +
"NativeScript CLI might not work properly.$lineSeparator" +
"Remove applicationId from app.gradle and update the \"nativescript.id\" in package.json.$lineSeparator" +
"Actual: ${android.defaultConfig.applicationId}$lineSeparator" +
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

@ns-bot
Copy link

ns-bot commented Jan 30, 2018

💚

@KristianDD
feat(build.gradle): take applicationId from package.json
24e77aa 
fix(build.gradle): warning for different indentifiers not shown
@KristianDD KristianDD force-pushed the kddimitrov/application-id-taken-from-packagejson branch from 248332f to 24e77aa Compare April 24, 2018 16:25
vtrifonov
vtrifonov approved these changes May 10, 2018
View reviewed changes
test-app/app/build.gradle Outdated
def getAppIdentifier = { packageJsonMap ->
def appIdentifier = "";
if (packageJsonMap) {
appIdentifier = packageJsonMap.nativescript.id;
Copy link
Contributor

Choose a reason for hiding this comment

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

what about testing for packageJsonMap.nativescript

@vtrifonov vtrifonov merged commit 81be198 into master May 10, 2018
@vtrifonov vtrifonov deleted the kddimitrov/application-id-taken-from-packagejson branch May 10, 2018 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtrifonov vtrifonov vtrifonov approved these changes

@Plamen5kov Plamen5kov Plamen5kov approved these changes

@petekanev petekanev petekanev approved these changes

Assignees

@KristianDD KristianDD

Labels
in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@KristianDD @Plamen5kov @ns-bot @petekanev @vtrifonov