feat(client): client/src/cordova/dev action #2111
Conversation
daniellacosse
changed the title
daniellacosse
force-pushed
the
daniellacosse/ui_reload
branch
from
bc8dc50
to
41177cc
Compare
|
note to self: revert package.json changes DONE |
client/src/www/index_cordova.html
Outdated
| @@ -26,5 +26,16 @@ | |||
| </head> | |||
| <body unresolved> | |||
| <app-root></app-root> | |||
|
|
|||
| <script> | |||
There was a problem hiding this comment.
Unfortunately I can't inject this in debug mode because that mode doesn't work on MacOS.
@fortuna are there security concerns with this?
There was a problem hiding this comment.
If it's a problem, I can try injecting this script tag in the action after cordova/src/www/build is run! Maybe with jsdom?
There was a problem hiding this comment.
Could you use a separate index_cordova.dev.html file and use that in the cordova setup using makeReplacements()?
There was a problem hiding this comment.
So creating a separate file would mean i'd have to breakout a whole other webpack config/build workflow so instead i opted to just call makeReplacements() on the index_cordova.html output by the webpack process. PTAL.
daniellacosse
changed the title
client/src/cordova/dev.action.mjs
Outdated
| }; | ||
|
|
||
| /** | ||
| * @description Builds the parameterized cordova binary (ios, macos, maccatalyst, android). |
There was a problem hiding this comment.
Did you push your latest changes?
There was a problem hiding this comment.
Oops! (pushed)
client/src/cordova/dev.action.mjs
Outdated
| export async function main() { | ||
| console.warn( | ||
| chalk.yellow( | ||
| 'This action only works for the MacOS platform. Ignoring all inputs.' |
There was a problem hiding this comment.
Why not parse the inputs and exit early or throw an error if it's not targeting macos? It would be more explicit.
| 'This action only works for the MacOS platform. Ignoring all inputs.' | ||
| ) | ||
| ); | ||
| export async function main(...givenParameters) { |
There was a problem hiding this comment.
Nice, thanks. This is better.
|
|
||
| <script> | ||
| try { | ||
| const reloadSocket = new WebSocket("ws://localhost:35729"); |
There was a problem hiding this comment.
Do you need to hard-code the server? Can this be a dynamic port of a unix socket instead?
You start the reload server, so you might as well use a dynamic port to avoid port conflicts.
| ); | ||
|
|
||
| return true; | ||
| }).listen(RELOAD_SERVER_PORT); |
There was a problem hiding this comment.
I believe you are listening on the "any" address here. This is a security issue. Please change this to localhost.
Also, use port zero, so that the OS assign a port for you. Then read the port (really, the full address) to pass it to the websocket url.
There was a problem hiding this comment.
Thanks for the tips - this is great. Since we merged this I'll do it in a follow up PR ASAP.
simply run
npm run action client/src/cordova/devto start working in a native livereload environment!demo (note that I'm speaking throughout):
lq-demo.mov