Add :run-options config #10
Conversation
|
This required for status-im/status-mobile#5304 |
src/clj_rn/core.clj
Outdated
| @@ -178,10 +178,16 @@ | |||
| (future (shell/exec full-command)) | |||
| (shell/exec full-command))))) | |||
|
|
|||
| (defn get-run-options | |||
| [build-id config] | |||
| (or (get-in config [:platforms build-id :run-options]) (:run-options config))) | |||
There was a problem hiding this comment.
How about this?
(defn- get-platform-config
[config platform]
(merge config (get-in config [:platforms platform])))
(defn- run-builds
[build-ids config]
(doseq [build-id build-ids]
(let [platform-config (get-platform-config config build-id)
command (str "run-" (name build-id) " " (:run-options platform-config))]
(execute-react-native-cli command))))
There was a problem hiding this comment.
i'm not sure if merge works for strings, and see Julien comment below
There was a problem hiding this comment.
It doesn't, but I don't merge strings here. Platform config is a map.
clj-rn.conf.example.edn
Outdated
| :run-options "" | ||
|
|
||
| ;; platform specific config. Supported only `:run-options` | ||
| :platform {:android {:run-options "--appPrefix debug"}}} |
There was a problem hiding this comment.
How about having :run-options {:android "" :ios "" :default ""} ?
Also do platform specifics override the default?
There was a problem hiding this comment.
reasons why I made it like this:
- It is similar to re-natal
- This PR Support platform-dependent modules list in clj-rn.conf.edn #9 require it
- It is easier to specify default config. Just
:run-options "something" - It is easier to work with it. Just merge platform config to main config
(merge config (get-in config [:platforms platform]))
Platform specific config overrides default.
There was a problem hiding this comment.
From a user perspective having a single option makes more sense. Would this prevent the other PR?
There was a problem hiding this comment.
It requires more work to do it like this if it should be similar for all options and I need to change pr for status-react. If only for :runs-options then it is easier.
There was a problem hiding this comment.
if it should be similar for all options
What do you mean?
There was a problem hiding this comment.
It is for the future so all options would look the same. If it is not required now I can do it only for :run-options.
There was a problem hiding this comment.
Yes let's try to find out what the best solution would be to be future proof. Then only implement it for run-options
There was a problem hiding this comment.
ok. So I do :run-options like this :run-options {:android "" :ios "" :default ""}?
There was a problem hiding this comment.
I think that's the cleanest way to do this?
There was a problem hiding this comment.
ok, I will do it like this.
src/clj_rn/shell.clj
Outdated
| @@ -10,7 +10,7 @@ | |||
|
|
|||
| (defn exec | |||
| [command] | |||
| (let [process (.exec (Runtime/getRuntime) (into-array (str/split command #" ")))] | |||
| (let [process (.exec (Runtime/getRuntime) (into-array ["sh" "-c" command]))] | |||
There was a problem hiding this comment.
Can you detail this? How will windows be impacted here?
There was a problem hiding this comment.
Runtime/getRuntime require command as an array.
This command
command run --first-arg="something here" --second-arg="something here" --last-arg
should be separated like this
command
run
--first-arg="something here"
--second-arg="something here"
--last-arg
I can split string with this regexp \s+(?=(?:[^\"]*\"[^\"]*\")*[^\"]*$) if it should work on windows.
endenwer
force-pushed
the
feature/run-options
branch
from
ccd60df
to
d59dbdb
Compare
|
@jeluard I changed |
|
Awesome @endenwer ! I just merged it! |
Add ability to set options for
react-native run-*.