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

Fix local frontend file watching #2088

Merged
merged 3 commits into from Aug 16, 2018

Conversation

Projects
None yet
3 participants
@tadhg-ohiggins
Collaborator

tadhg-ohiggins commented Aug 16, 2018

We must be watchful.
But who watches the watchmen?
It's complicated…

Fix for #2087.

This turned out not to be simply that the SASS files weren't being watched, but also that the gulp build wasn't entirely finishing, as could be seen by the lack of the Visit your CALC at: line in the console output.

(Problems listed in order of discovery.)

Problem 1: The second argument to gulp.watch now has to be a function, not an array of function names.

Fixed by making gulp.watch invocations use gulp.series and/or gulp.parallel.

Problem 2: Putting the set-watching task in the set of arguments to gulp.parallel meant that it didn't finish executing first, which seemed to be necessary to truly finish the watch task (it would claim in the console output that it finished, but it wouldn't show the Visit your CALC at: line).

Fixed by surrounding the gulp.parallel call with a gulp.series call that runs set-watching first and then runs the rest in parallel.

Problem 3: Apparently older versions of gulp would regard tasks as finished if they returned (I don't know that this is true but am going by how the older code was written), but now they need to call a callback to register that they've finished. The set-watching task's function didn't have that, so gulp would never see it had finished.

Fixed by adding the callback as an argument to the set-watching task and then calling it.

Problem 4: Similar to Problem 3 but wrapped in significantly more layers of indirection, the webpackify function in frontend/gulp/webpack-util.js also needs to call a callback to let gulp know that it has finished. Without this, it would never complete, leading to gulp (and possibly webpack also?) not watching for changes in the files it was supposed to be watching.

Fixed by adding a callback argument to the call to webpack-stream (because things weren't complicated enough already so we need a wrapper around webpack that plays nicely with gulp) and then a function that calls the callback if a new global variable, taskNum, equals 1 (apparently we don't want to run the callback outside of initial startup). I don't know enough about webpack or about what the intended behavior is here to be certain that webpack is now doing exactly what we want.
But after fixing this the Visit your CALC at: line appears in the console output, which seems to signify that gulp is actually doing everything that it's supposed to do for us, and changing a .sass file results in gulp re-running part of the build and the local server, thus reflecting the changes made in that file without needing to restart.
Hence I am declaring victory.

Tadhg O'Higgins added some commits Aug 16, 2018

Tadhg O'Higgins
Fix four layers of problems that caused local frontend file watching …
…not to function. See PR for gory details.
Tadhg O'Higgins

@tadhg-ohiggins tadhg-ohiggins requested a review from hbillings Aug 16, 2018

@tadhg-ohiggins tadhg-ohiggins changed the title from Fixlocal frontend file watching to Fix local frontend file watching Aug 16, 2018

@tbaxter-18f

This comment has been minimized.

Member

tbaxter-18f commented Aug 16, 2018

Ugh. I thought I'd worked through all the gulp 4 issues here: 0dd1bd5

This may be a good time to remove the bit in config.yml (in that commit) that's uninstalling gulp and installing gulp4 as part of the build, too.

@hbillings

This is some excellent sleuthing! All the high fives to you, sir.

@hbillings

This comment has been minimized.

Member

hbillings commented Aug 16, 2018

@tadhg-ohiggins Normally my stance is to let folks merge their own PRs, but since I'm trying to get a prod release out today, I'm just going to merge this now.

@hbillings hbillings merged commit b411108 into develop Aug 16, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
security/snyk - package.json (CALC) No manifest changes detected
security/snyk - requirements.txt (CALC) No manifest changes detected

@hbillings hbillings deleted the 2087-fix-local-frontend-file-watching branch Aug 16, 2018

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