Build callback not fired with `watch()` + async plugins about metalsmith HOT 13 CLOSED

Thx for the repro but I just cloned the main govuk-design-system on your stuck branch, and was able to reproduce the fix.
FWIW injectChanges: true and clean(false) also worked (=partial reload). I'll make sure to test against this repo before releasing

from metalsmith.

Hi there, and thanks for creating an issue for this. I've done rigorous testing using async plugins (most plugins are async) and think you are onto something, but it's definitely not with async plugins in general as it works for https://github.com/metalsmith/metalsmith.io. I did have to finetune the browsersync opts quite a bit. Have a look at how the build function runs in: https://github.com/metalsmith/metalsmith.io/blob/master/metalsmith.js#L212.

In your setup, you instructed browser-sync to watch the paths in option files. That is kind of suboptimal because now metalsmith attaches watchers to all your source files, and browser-sync attaches watchers to the build output to reload. In the metalsmith.io setup I do not use the browser-sync watcher, and instead reload immediately when the metalsmith build has rerun

These are the relevant lines.

let devServer

ms.clean(isProduction)
  .watch(isProduction ? false : ['src', 'lib'])
  .build(err => {
    if (err) {
      throw err;
    }
    if (ms.watch()) {
      if (devServer) {
        devServer.reload();
      } else {
        devServer = browserSync.create();
        devServer.init({
          host: 'localhost',
          server: './build',
          port: 3000,
          injectChanges: false,
          reloadThrottle: 0
        });
      }
    }
  });

from metalsmith.

@webketje Ah that's great for injectChanges: false but we'd like to reload on build output changes

For example if we've typed into some <form> fields we want the CSS to reload not the page

bs.init({
  files: [
    join(paths.public, 'javascripts/**/*.js'),
    join(paths.public, 'stylesheets/**/*.css')
  ],

  // ...
})

Were you able to see that the build callback didn't run? Seems to be caused by these async plugins:

https://github.com/alphagov/govuk-design-system/blob/browsersync-reload-stuck/lib/metalsmith.js#L168-L175

// build the entrypoint for application specific JavaScript
.use(rollup('javascripts/application.mjs'))

// build GOV.UK Frontend JavaScript
.use(rollup('javascripts/govuk-frontend.mjs'))

// build the entrypoint for example specific JavaScript
.use(rollup('javascripts/example.mjs'))

from metalsmith.

No I haven't had the opportunity to pull in your repo yet. The point of the snippet in my previous comment is devServer.reload() and not injectChanges (both should work). It makes little sense to use both the browsersync & metalsmith watchers (you need to know 1) when the source changes -> metalsmith watcher , and 2) when to reload browser -> in metalsmith build callback. There is no utility for the browsersync-bundled watcher (prefer browserSyncInstance.reload() as in the snippet).

Can you try that? (so, removing the browserSync "files" option, not using the browsersync watcher and calling devServer.reload() manually in metalsmith.build (which should achieve the same). IMO browserSync.reload should also work with injectChanges: true

from metalsmith.

Thanks @webketje

I've pushed again with those suggestions but sadly the build() callback still won't fire

BUT I've possibly found something

When I turn off minification (Terser, quite slow) from our Rollup plugin config build() fires 🎉
When I reintroduce additional delays of 50ms then build() stops firing again

const { setTimeout } = require('timers/promises')

await setTimeout(25) // ✅ Metalsmith `build()` called
await setTimeout(50) // ✅ Metalsmith `build()` called
await setTimeout(75) // ❌ Metalsmith `build()` NOT called
await setTimeout(100) // ❌ Metalsmith `build()` NOT called

from metalsmith.

Ok I will look into it (and clone the branch you linked to to investigate). The execution duration of a plugin should not matter, as long as it resolves after all actions have completed using done() or Promise.resolve(). I also had a look at the source code of @rollup/plugin-terser and my guess is this maybe has to do with Node worker threads (it's almost the only plugin using it, and I haven't tested metalsmith plugins relying on workers yet).

from metalsmith.

I've hit another interesting, related issue: on Windows, the chokidar 'ready' event seems not to fire at all (regardless of ignoreInitial). While going through its source code I noticed chokidar's ready event is added on process.nextTick.

3 approaches worked:

• setting ignoreInitial: false in chokidar options in the metalsmith watcher
• moving the first metalsmith build outside of watcher.on('ready')
• launching browsersync on process.nextTick inside the build callback

So this is the temporary workaround:

let devServer

ms.clean(isProduction)
  .watch(isProduction ? false : ['src', 'lib'])
  .build(err => {
    if (err) {
      throw err;
    }
    if (ms.watch()) {
      process.nextTick(() => {
        if (devServer) {
          devServer.reload()
        } else {
          devServer = browserSync.create();
          devServer.init({
            host: 'localhost',
            server: './build',
            port: 3000,
            injectChanges: false,
            reloadThrottle: 0
          });
        }
      })
    }
  });

from metalsmith.

Thanks @webketje

My build() callback doesn't actually get called so I'm not sure how I can use the temporary workaround?

I did try 1) as the docs for watch() show I can pass in Chokidar.WatchOptions but { ignoreInitial: false } is overridden by design so I couldn't use it without editing your package code:

Alternatively an object of Chokidar watchOptions, except cwd, ignored, alwaysStat, ignoreInitial, and awaitWriteFinish. These options are controlled by Metalsmith.

Shall I put together a more minimal example?

from metalsmith.

@colinrotherham

Alternatively an object of Chokidar watchOptions, except cwd, ignored, alwaysStat, ignoreInitial, and awaitWriteFinish. These options are controlled by Metalsmith.

Good spot 👍. I intended this to be available, but forgot to actually allow it in the source code (this is due to the fragmented development span of the watch feature + lack of time for unit testing, hence why I marked it experimental).

My build() callback doesn't actually get called so I'm not sure how I can use the temporary workaround?

So I'm fairly confident the issue is that watcher.on('ready' gets attached after the ready event has already fired, resulting in the first build never being run (because it is run on watcher.on('ready'). The watcher always does a first run (~Metalsmith.process) but does not execute the build callback (including write to disk). process.nextTick alters execution order (I'm unsure in this case how) and somehow results in the first build running anyway.

I'll work on a patch release ASAP

from metalsmith.

Appreciate it @webketje

I missed your edit initially so here's a minimal demo repo:
https://github.com/colinrotherham/metalsmith-async-watch/blob/main/index.mjs#L1-L20

import { dirname, join } from 'node:path'
import { setTimeout } from 'node:timers/promises'
import { fileURLToPath } from 'node:url'
import Metalsmith from 'metalsmith'

const { NODE_ENV = 'production' } = process.env
const basePath = dirname(fileURLToPath(import.meta.url))

Metalsmith(basePath)
  .source(join(basePath, 'src'))
  .destination(join(basePath, 'dist'))

  .watch(NODE_ENV === 'development')
  .clean(NODE_ENV === 'production')

  // Run slow async plugin
  .use(() => setTimeout(1000))

  // Callback not logged due to slow async plugin
  .build(() => console.log('Build complete'))

I'll work on a patch release ASAP

Thanks again. Happy to test

from metalsmith.

Hi @webketje I've done some testing of the fix this morning but the build() callback still doesn't run

You can see Metalsmith installed from github:metalsmith/metalsmith#master in:

• alphagov/govuk-design-system@9037231

from metalsmith.

I cloned the browsersync-reload-stuck branch and ran it locally (both on Windows 11 & Linux 16G RAM machines) and the initial build callback always runs, maybe do a clean clone & install + clean public folder? The ms rebuild is activated almost instantly after subsequent change, the rebuild + browser-sync reload take about 3-6s.

Mind that I explicitly added warnings the watcher was experimental as I knew this would open up a Pandora's box of potential cross-platform & plugin repeat run issues and didn't have enough time to thoroughly test it, but your early feedback is very much appreciated

I did hit other issues on Windows (which merit their own GH issues):

• VS Code /Codium uses a file watcher that is activated by default, and is also based on chokidar. in 50% of cases an edit via VS Code/ Codium triggered 2 simultaneous builds, which lead to plugin errors. Confirmed this doesn't happen through an editor without built-in file watching (eg Notepad, Notepad++)
• The fix I included in master works a charm & speeds up auto-reload considerably on Linux, but causes quickly succeeding file changes not to be batched, leading to plugin errors.
• Bugs may be caused by unadressed issues (+100) in the underlying chokidar lib
• Bugs may be caused by custom or third-party plugins altering the metalsmith build in unorthodox ways (and this is not to blame users, rather lack of docs). The watcher should be more resilient in these cases but I suspect govuk does have some of these (for example what's in lib/file-helper.js could probably be achieved in a cleaner way + does a lot of sync calls which slow the entire build)

from metalsmith.

@webketje Yeah we've got lots of slow code

Hopefully we're the perfect example 😊

Ah I'm on a Mac so that could be something. Chokidar uses File System Events (FSEvents) on macOS, maybe there's just enough of a difference for us not to see that build() handler run

We'll test the release when it's out

Thanks again

from metalsmith.