Change to decoupled binary config format #268
Conversation
DanielMSchmidt
force-pushed
the
config/decouple-build-path-from-config
branch
from
49b7f3b
to
dec4d03
Compare
|
This diff is huge, all the reformatting make it very hard to actually go over the code. Couldn't conclude if there are any breaking changes, but if there are, let's introduce them all together and not bit by bit. |
|
@rotemmiz updated the diff to be a lot cleaner. This way it would be a breaking change, but I am currently working on a solution to not introduce a breaking change
|
DanielMSchmidt
force-pushed
the
config/decouple-build-path-from-config
branch
3 times, most recently
from
799e00a
to
c0812ff
Compare
|
I just added a second commit which infers the appName if it's not set. This leads to
I will think about another way to refer to these fallbacks as late as possible so that we don't break detox for anyone, haven't got a clear idea yet. |
DanielMSchmidt
force-pushed
the
config/decouple-build-path-from-config
branch
4 times, most recently
from
bc9b285
to
799d7da
Compare
|
@rotemmiz This is a more final version, it should not break the usage for anyone, still lets not merge it yet. Still thinking about a clean way to support the current version without relying on the automatic finding of the app name to work |
This allows us to have a less verbose config, tackling the first part of #175. This will introduce a breaking change, we might want to add a warning first instead of an error.
To reduce configuration for our users we can now find out how their app is named
This is useful because it helps us not to break anything for current users with hard coded binary fields
DanielMSchmidt
force-pushed
the
config/decouple-build-path-from-config
branch
from
08ffb16
to
52eb2d5
Compare
| } | ||
|
|
||
| const binaryPaths = this.userConfig.binary || {}; | ||
| const defaultBinaryPathOverwrite = binaryPaths[deviceDriver.getPlatform()]; |
There was a problem hiding this comment.
Instead, why not use deviceDriver.getBinaryPath() ?
|
|
||
| const androidFileContent = ` | ||
| <manifest xmlns:android="http://schemas.android.com/apk/res/android" | ||
| package="com.myAndroidApp" |
There was a problem hiding this comment.
This is not necessarily the final package name of the app. The source of truth is in the app/build.gradle.
| const deviceDriver = new deviceClass(this.client); | ||
| this.device = new Device(deviceConfig, sessionConfig, deviceDriver); | ||
| const binaryPath = await this._getBinaryPath(deviceConfig, deviceDriver); |
There was a problem hiding this comment.
BinaryPath should be contained inside deviceDriver IMO
| await this.device.prepare(params); | ||
| global.device = this.device; | ||
| } | ||
|
|
||
| async _getAppName(deviceDriver) { |
There was a problem hiding this comment.
I have a feeling that trying to extract the appName from project files will get us in lots of trouble. There are endless possibilities for configuring projects (especially if there are multiple targets on iOS or multiple buildFlavors on Android, and build.gradle might be in custom directories (not necessarily in /app/
This allows us to have a less verbose config, tackling the first part of #175.
This will introduce a breaking change (
appNamefield), we might want to add a warning first instead of an error.