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

feat: make client port configurable through environment variable #352

Merged
merged 1 commit into from Sep 18, 2018

Conversation

Projects
None yet
4 participants
@PascalPrecht
Copy link
Contributor

PascalPrecht commented Sep 11, 2018

No description provided.

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:feat/client-port-env-var branch from da69e66 to 8cdc713 Sep 11, 2018

@@ -75,7 +75,7 @@
},
"scripts": {
"ui-assets": "copy-aragon-ui-assets -n aragon-ui ./build",
"start": "npm run ui-assets && parcel serve src/index.html --port 3000 --out-dir ./build --no-cache",
"start": "npm run ui-assets && parcel serve src/index.html --port $ARAGON_CLIENT_PORT || 3000 --out-dir ./build --no-cache",

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 11, 2018

Contributor

As @izqui pointed out offline, this expression doesn't actually work in Bash. Only test when $ARAGON_CLIENT_PORT was set.

Will update.

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:feat/client-port-env-var branch from 8cdc713 to 9dd898c Sep 11, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 11, 2018

Bash makes it really hard to read and repetitive as well.. Another way to go about this is using something like if-env. Let me know if you want me to give this a spin.

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Sep 11, 2018

Maybe we could move the start script to scripts/start.sh? Or even to a node script, for Windows compatibility.

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 11, 2018

Oh yea, totally. I like that

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:feat/client-port-env-var branch from 9dd898c to 52fa04b Sep 11, 2018

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Sep 11, 2018

It looks like cross-var could be viable too, but it's not widely used and a node script might not be much more effort (and would pave the way for the scripts/start_local.sh to be converted too).

For future reference though, parameter substitution might've been nicer in the script (could just use ${ARAGON_CLIENT_PORT:-3000}).

@@ -0,0 +1,8 @@
#!/usr/bin/env bash

if [[ ${ARAGON_CLIENT_PORT} ]];

This comment has been minimized.

@sohkai

sohkai Sep 11, 2018

Member

Wondering if we should add a REACT_APP prefix, given that all the other env vars use it.

Alternatively, we could change all the other env vars to not use that prefix since we don't need it anymore after moving to parcel.

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 12, 2018

Contributor

Wondering if we should add a REACT_APP prefix, given that all the other env vars use it.

I don't have super strong feelings about it, just that I think that bit of information, whether it's a react app or not, is not really necessary in that env variable.

But I'm happy to change it regardless.

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 12, 2018

Contributor

Actually, on a second thought, I have to say I probably should've stayed consistent with how env variables have been named in the past. I didn't know REACT_APP_* vars were related to the wrapper client.

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 12, 2018

For future reference though, parameter substitution might've been nicer in the script (could just use ${ARAGON_CLIENT_PORT:-3000}).

Ha, that's awesome! Thanks for pointing this out.

I guess I'll convert this script to node then and update the PR

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:feat/client-port-env-var branch from 52fa04b to 30d03f2 Sep 12, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 12, 2018

Alright, this is done. PTAL and let me know if you want to change the env variable.

@sohkai

sohkai approved these changes Sep 12, 2018

Copy link
Member

sohkai left a comment

❤️ Looking good!

@@ -0,0 +1,7 @@
const execute = require('child_process').execSync

This comment has been minimized.

@sohkai

sohkai Sep 12, 2018

Member

Should we add the #!/usr/bin/env node here?

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 12, 2018

Contributor

Actually, this is already a JS file and executed with node so I don't think this is needed. Unless, of course, you say you want this executable as a plain text file.

This comment has been minimized.

@sohkai

sohkai Sep 12, 2018

Member

Might be nice to include that as an option, in case someone ever does want to.

I'd find it weird to have to use node scripts/start.js rather than just scripts/start.js directly.

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 12, 2018

Contributor

Ah sure. I thought this was rather a thing to do when you do indeed have a plain text file (without .js extension). I'll update it :)

@@ -0,0 +1,7 @@
const execute = require('child_process').execSync

const clientPort = process.env.ARAGON_CLIENT_PORT || 3000

This comment has been minimized.

@sohkai

sohkai Sep 12, 2018

Member

I think for now it's easiest to stay consistent and use the REACT_APP prefix, and then we can wholesale remove it from both this repo and aragon-cli in one go after.

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 12, 2018

Thanks for the review @sohkai ! I'll update this.

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:feat/client-port-env-var branch from 30d03f2 to 228d329 Sep 12, 2018

feat: make client port configurable through environment variable
This commit also moves the npm start script into scripts/start for
better readability and maintainability.

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:feat/client-port-env-var branch from 228d329 to 25211a5 Sep 13, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 13, 2018

@sohkai updated it once more. Notice that I've also dropped the .js extension and went with a plain file instead. This felt more natural to me for this use case and lets you run ./scripts/start.

Let me know if you want to keep the .js extension.

@izqui

izqui approved these changes Sep 13, 2018

@sohkai

sohkai approved these changes Sep 13, 2018

Copy link
Member

sohkai left a comment

Looks good!

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Sep 18, 2018

Good to merge @bpierre?

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Sep 18, 2018

@izqui I wonder how the execute() would work on WIN environments, but we can see that later. LGTM 👍

@izqui izqui merged commit 66f86ff into aragon:master Sep 18, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@PascalPrecht PascalPrecht deleted the PascalPrecht:feat/client-port-env-var branch Sep 18, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 18, 2018

Happy to see this landed! @bpierre I haven't tested this on Windows, but I also couldn't find any active issues on execSync with Windows. Anything in particular you're aware of?

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Sep 18, 2018

@PascalPrecht Cool! No it’s just that I’m not very familiar with the Windows CLI: I know Unix path separators are accepted, but I was wondering if && and stdio: 'inherit' would work the same way.

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 18, 2018

Got it. I guess this needs to be tested then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment