feat: Implement livereloading for wrangler preview #252

xortive · 2019-06-14T16:37:26Z

this PR adds the --watch flag to build and preview, which watches your Wrangler project for changes and builds it when it's ready. It works for all project types

javascript -> watches the entry point in package.json
rust -> watches the /src directory, and the entry point in package.json
webpack -> uses webpack's watching, which just works

When run in --watch mode, preview hides the fiddle editor, since you would be using a local text editor instead.

to test:
cargo install --force --debug --git https://github.com/xortive/wrangler --branch malonso/hotreload

Closes #235

src/commands/build/wranglerjs/mod.rs

wranglerjs/index.js

src/commands/build/wranglerjs/mod.rs

xortive · 2019-07-09T22:04:18Z

This is feature complete!

Tagging @ejcx for security review on the wrangler side of this, specifically directed towards the websocket portion of this PR.

xortive · 2019-07-10T15:17:27Z

src/commands/build/watch/mod.rs

+use std::thread;
+use std::time::Duration;
+
+pub const COOLDOWN_PERIOD: Duration = Duration::from_millis(2000);


Considering making this a config option, it refers to the amount of time wrangler will ignore new fs events when it detects the first one. Any new events during the cooldown period restart the timer.

xortive · 2019-07-10T22:10:32Z

~~the ctrl-c handler doesn't work after the notifier detects it's first change. Have no idea where to even begin debugging something like this~~

just going to revert, os should clean the temp file in the temp dir on reboot anyways so we shouldn't need an explicit ctrlc handler

src/commands/publish/preview/mod.rs

xortive · 2019-07-15T15:32:33Z

I'm mildly concerned about the stability of this feature, and I think maybe we should include it in a RC release first or gate it under an experimental flag. The worst thing that can happen however is a poor user experience -- the watcher not seeing an update or wrangler panicing since it expects a file to be present that is not present.

Especially with the webpack project type that effectively has two file watchers connected together (webpack watches project, we watch webpack output) there may be a race condition or two lingering that could cause a panic. Better error handling or tests for livereloading would mitigate this concern.

kristianfreeman · 2019-08-15T19:25:59Z

this 502s for me locally :( unclear if it's related to #429, what would be useful for debugging here?

☁  worker  ~/.cargo/bin/wrangler preview --watch
⬇️ Installing wasm-pack...
✨   Built successfully, built project size is 2 KiB.
Error: https://cloudflareworkers.com/script: Server Error: 502 Bad Gateway

update: talked to @ashleygwilliams about this, the project is 502ing because it has KV bindings in wrangler.toml. keeping this project around for QA tomorrow...

kristianfreeman · 2019-08-15T19:36:23Z

this worked really well for me with a new application, nice work @xortive!

one piece of feedback: as i watched the live reloading happen in the terminal, i found that it was unclear when the browser should be up-to-date. here's the terminal output i saw a few times as i made changes:

🌀  Detected changes...
🌀  Detected change during cooldown...
🌀  Cooldown over, propogating changes...
✨   Built successfully, built project size is 570 bytes.
👷  Updating preview with changes

it seems like the "Updating preview with changes" step is always the last thing that happens before the preview actually, well, updates. would it make sense to change that text output to "Updated preview with changes", or have another line of output that indicates when the preview window is done updating? not a blocker, just stood out to me as i was trying this out

EverlastingBugstopper · 2019-08-16T14:11:21Z

src/commands/publish/preview/mod.rs

            &session.to_string(), ws_port, script_id, https_str, preview_host,
        ))?;

+        //don't do initial GET + POST with livereload as the expected behavior is unclear.


what do you mean here?

we ignore the wrangler preview get --watch and wrangler preview post --watch commands as it is unclear what they should do. They will be addressed once we have a command for browserless preview

EverlastingBugstopper

This is looking really good with the new changes to authenticated preview! Want to wait for @ashleymichal and @signalnerve to review w/their KV projects before merging.

I think Kristian is going to take on writing up a section in the README.

One more todo for @xortive - ~~please hide the sidebar for wrangler preview, this is looking awesome awesome!!~~ just merge this PR xortive#1

kristianfreeman · 2019-08-16T14:54:23Z

my previous issue w/ wrangler preview --watch on a project with kv has been fixed - yay! i'll work on some readme documentation, will track it in a separate PR

Co-Authored-By: Sven Sauleau <github@sauleau.com>

EverlastingBugstopper

lgtm!

own logic for picking a port

ashleygwilliams

<3

src/commands/build/watch/mod.rs

ashleygwilliams · 2019-08-16T20:14:38Z

src/commands/build/watch/watcher.rs

+            Ok(Some(path)) => {
+                message::working("Detected changes...");
+                //wait for cooldown
+                while let Ok(_e) = rx.recv_timeout(cooldown) {


convention here would be to just write Ok(_)

ashleygwilliams · 2019-08-16T20:18:05Z

src/commands/build/wranglerjs/mod.rs

+    project: &Project,
+    tx: Option<Sender<()>>,
+) -> Result<(), failure::Error> {
+    let (mut command, temp_file, bundle) = setup_build(project)?;


long term we probably want a better solution here ;)

ashleygwilliams · 2019-08-16T20:19:16Z

src/commands/build/wranglerjs/mod.rs

+        let _command_guard = util::GuardedCommand::spawn(command);
+
+        let (watcher_tx, watcher_rx) = channel();
+        let mut watcher = watcher(watcher_tx, Duration::from_secs(1)).unwrap();


i am worried about these unwraps!

if we can: let's get rid of unwraps- make them errors if at all possible. if not possible: info debugging lines would be very useful. ask yourself- when this panics.. what am i gonna want to know

(this is basically creating an expect)

src/commands/build/wranglerjs/mod.rs

src/commands/publish/preview/upload.rs

ashleygwilliams · 2019-08-16T20:28:12Z

src/main.rs

@@ -188,10 +200,18 @@ fn run() -> Result<(), failure::Error> {
            None => None,
        };
        commands::init(name, project_type)?;
-    } else if matches.subcommand_matches("build").is_some() {
+    } else if let Some(matches) = matches.subcommand_matches("build") {


do we need build --watch?

ashleygwilliams · 2019-08-16T20:29:31Z

src/main.rs

@@ -207,7 +227,12 @@ fn run() -> Result<(), failure::Error> {
            None => None,
        };

-        commands::preview(project, user, method, body)?;
+        let watch = match matches.occurrences_of("watch") {


match presence of not occurences .. because what if someone types wrangler preview --watch --watch

ashleygwilliams · 2019-08-16T21:10:34Z

closing in favor of #451

xortive mentioned this pull request Jun 17, 2019

give option to not open browser when previewing #256

Closed

xortive force-pushed the malonso/hotreload branch 2 times, most recently from 8207886 to 0ac995e Compare June 19, 2019 16:31

ashleygwilliams added work in progress labels Jun 20, 2019

ashleygwilliams added this to the 1.1.0 milestone Jun 20, 2019

xtuc reviewed Jun 25, 2019

View reviewed changes

src/commands/build/wranglerjs/mod.rs Outdated Show resolved Hide resolved

xtuc reviewed Jun 25, 2019

View reviewed changes

wranglerjs/index.js Show resolved Hide resolved

xtuc reviewed Jun 25, 2019

View reviewed changes

src/commands/build/wranglerjs/mod.rs Outdated Show resolved Hide resolved

xortive force-pushed the malonso/hotreload branch from bb9d992 to 68972cb Compare June 25, 2019 20:03

ashleygwilliams added the blocked label Jul 1, 2019

xortive removed this from the 1.1.0 milestone Jul 1, 2019

xortive self-assigned this Jul 1, 2019

ashleymichal removed blocked labels Jul 8, 2019

xortive force-pushed the malonso/hotreload branch 3 times, most recently from 4b9b39a to 0e0034d Compare July 9, 2019 17:04

xortive requested a review from ejcx July 9, 2019 22:02

xortive changed the title ~~WIP: feat: Implement livereloading for wrangler preview~~ feat: Implement livereloading for wrangler preview Jul 9, 2019

xortive added status - needs review and removed status - work in progress labels Jul 9, 2019

xortive requested a review from xtuc July 9, 2019 22:05

xortive force-pushed the malonso/hotreload branch from f63473e to ed860ed Compare July 10, 2019 15:14

xortive commented Jul 10, 2019

View reviewed changes

ejcx reviewed Jul 12, 2019

View reviewed changes

src/commands/publish/preview/mod.rs Outdated Show resolved Hide resolved

xortive force-pushed the malonso/hotreload branch from 7bb07f6 to 4b586ef Compare July 15, 2019 15:09

xortive added 2 commits August 15, 2019 14:54

update preview service url

cb60448

Merge remote-tracking branch 'origin/master' into malonso/hotreload

20cbfe9

EverlastingBugstopper reviewed Aug 16, 2019

View reviewed changes

Merge branch 'master' into malonso/hotreload

694d078

EverlastingBugstopper requested review from EverlastingBugstopper, ashleygwilliams, ejcx, gabbifish and xtuc August 16, 2019 14:40

EverlastingBugstopper reviewed Aug 16, 2019

View reviewed changes

xortive and others added 2 commits August 16, 2019 10:37

Use strict equality check in wranglerjs/index.js

930f920

Co-Authored-By: Sven Sauleau <github@sauleau.com>

Hide editor on wrangler preview without watch

73933fb

EverlastingBugstopper approved these changes Aug 16, 2019

View reviewed changes

xortive added 2 commits August 16, 2019 10:48

Better messaging for preview update message

ae50e37

use NonRecursive watcher for watch paths that always point to files

2f9d2dc

kristianfreeman mentioned this pull request Aug 16, 2019

Document live reload (and update NPM readme) #446

Merged

xortive added 2 commits August 16, 2019 11:14

bind to port 0 to allow the OS to assign us a port, instead of using our

ecb1ba9

own logic for picking a port

cargo fmt

61ed11e

xortive force-pushed the malonso/hotreload branch from 50d585e to b15fd59 Compare August 16, 2019 19:37

Make open_browser more robust

6e19a8e

xortive force-pushed the malonso/hotreload branch from b15fd59 to 6e19a8e Compare August 16, 2019 19:55

cargo fmt

fa4c023

ashleygwilliams reviewed Aug 16, 2019

View reviewed changes

xortive added 2 commits August 16, 2019 15:50

disable wrangler build --watch for now

23f1d99

PR feedback: namespacing, command matching, remove unnecessary unwraps

22c04df

ashleygwilliams closed this Aug 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement livereloading for wrangler preview #252

feat: Implement livereloading for wrangler preview #252

xortive commented Jun 14, 2019 •

edited

Loading

xortive commented Jul 9, 2019

xortive Jul 10, 2019

xortive commented Jul 10, 2019 •

edited

Loading

xortive commented Jul 15, 2019

kristianfreeman commented Aug 15, 2019 •

edited

Loading

kristianfreeman commented Aug 15, 2019

EverlastingBugstopper Aug 16, 2019

xortive Aug 16, 2019

EverlastingBugstopper left a comment •

edited

Loading

kristianfreeman commented Aug 16, 2019

EverlastingBugstopper left a comment

ashleygwilliams left a comment

ashleygwilliams Aug 16, 2019

ashleygwilliams Aug 16, 2019

ashleygwilliams Aug 16, 2019

ashleygwilliams Aug 16, 2019

ashleygwilliams Aug 16, 2019

ashleygwilliams Aug 16, 2019

ashleygwilliams Aug 16, 2019

ashleygwilliams commented Aug 16, 2019

feat: Implement livereloading for wrangler preview #252

feat: Implement livereloading for wrangler preview #252

Conversation

xortive commented Jun 14, 2019 • edited Loading

xortive commented Jul 9, 2019

Choose a reason for hiding this comment

xortive commented Jul 10, 2019 • edited Loading

xortive commented Jul 15, 2019

kristianfreeman commented Aug 15, 2019 • edited Loading

kristianfreeman commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EverlastingBugstopper left a comment • edited Loading

Choose a reason for hiding this comment

kristianfreeman commented Aug 16, 2019

EverlastingBugstopper left a comment

Choose a reason for hiding this comment

ashleygwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleygwilliams commented Aug 16, 2019

xortive commented Jun 14, 2019 •

edited

Loading

xortive commented Jul 10, 2019 •

edited

Loading

kristianfreeman commented Aug 15, 2019 •

edited

Loading

EverlastingBugstopper left a comment •

edited

Loading