Add processor cleanup function

[remcohaszing]

Yesterday I wrote remark-mermaidjs. Basically this code does the following:

function attacher() {
  return async function transformer() {
    // Start the browser for each file
    const browser = puppeteer.launch()

    // Do some stuff

    await browser.close()
  }
}

This works fine (well, not 100% consistent, but that’s a detail) in the tests.

I’m working on appsemble I have created the following script to generate JSX from the docs there:

import { promises as fs } from 'fs';
import { join } from 'path';

import mdx from '@mdx-js/mdx';
import { remarkMermaid } from 'remark-mermaidjs';

async function processPath(path) {
  const stats = await fs.stat(path);
  if (stats.isDirectory()) {
    const filenames = await fs.readdir(path);
    return Promise.all(filenames.map((f) => processPath(join(path, f))));
  }
  if (path.endsWith('.md')) {
    const content = await fs.readFile(path);
    const processed = await mdx(content, { remarkPlugins: [remarkMermaid] });
    fs.writeFile(`${path}.jsx`, processed);
  }
}

processPath('docs').catch((error) => {
  console.error(error);
});

This works as well.

In reality, I want to use this with @mdx-js/loader. This causes random failures due to puppeteer timeouts. I believe these timeouts are caused by the combination of Webpack bundling a fairly large project and opening Chrome 6 times in the background (once for each file that’s processed and contains mermaid graphs).

I think the solution would be if unified processorts get a cleanup function. Then I could deduplicate the heavy work in remark-mermaidjs, i.e. something like this:

function attacher() {
  // Start the browser once and reuse it
  const browserPromise = puppeteer.launch();

  this.addCleanup(() => browserPromise.then(browser => browser.close()));

  return async function transformer() {
    const browser = await browserPromise

    // Do some stuff

    await browser.close()
  }
}

[remcohaszing]

Oh and of course the processor would then have cleanup methods:

const processor = unified();

// $ExpectType Promise<void>
processor.cleanup();

// $ExpectType void
processor.cleanupSync();

[wooorm]

Not a fan of such a function, I think folks are going to forget it. It also doesn’t really fit with the unified process.

Instead, I’d suggest some of the following:

function plugin() {
  var queue

  return transform

  function transform(tree, file, next) {
    if (queue) {
      queue.push([tree, file, next])
    } else {
      queue = [[tree, file, next]]
      launch(..., onlaunch)
    }
  }

  function onlaunch(browser) {
    var [tree, file, next] = queue.shift()

    // ...do the work

    if (queue.length) {
      next() // This tree/file can go to the next plugin.
      onlaunch(browser) // Handle the next tree/file.
    } else {
      queue = undefined // Done!
      close(..., next)
    }
  }
}

It’s a lot of pseudo, and callbacks, but I hope you get the idea.

It essentially opens a browser, meanwhile queueing every transform. When the browser is ready, it flushes everything that’s queued. All the while more transforms can be queued, too. Finally, when there’s nothing in the queue anymore, the browser is closed.

Then, say a bit of time passes, and some more files are passed through, the whole thing starts itself up again.

[wooorm]

The cleanup function also opens up the problem of should happen if someone calls process after cleanup?

[remcohaszing]

Hmm, that's an interesting approach! I'll give it a try soon.

[ChristianMurphy]

Interesting @remcohaszing!
To help me understand, you want cleanup to happen after every call to unified.process and unified.run?
Or are you looking for a different timing?

[remcohaszing]

Because unified can only process one file at a time, I think calling the cleanup should happen manually. I.e. something like this:

const processor = remark().use(remarkMermaid);

// Start puppeteer once
await processor.process(await fs.readFile('CODE_OF_CONDUCT.md')).then(({ content }) => { console(content) });

// Reuse puppeteer
await processor.process(await fs.readFile('CONTRIBUTING.md')).then(({ content }) => { console(content) });

// Reuse puppeteer
await processor.process(await fs.readFile('LICENSE.md')).then(({ content }) => { console(content) });

// Reuse puppeteer
await processor.process(await fs.readFile('README.md')).then(({ content }) => { console(content) });

// Cleanup puppeteer
await processor.cleanup();

Another interesting approach would be if a transformer can process multiple files in one go. However, I don’t know how this would play out in @mdx-js/loader, since I’m not very familiar with how Webpack loaders work internally.

[ChristianMurphy]

This seems to be mostly related to multi-file processing.
/cc @wooorm I know you're doing some thinking/research into a layer on unified for handling many files.

[wooorm]

Yeah, I’m thinking a bunch about how that all would look. But there are so many layers... It’s a bit complex.
One similar case is where an RSS plugin would need access to ASTs of certain recent pages.
Or a sitemap.xml file.

There has been a feature out for ± 6 years, but it’s only supported on the engine: https://github.com/unifiedjs/unified-engine/blob/main/doc/plugins.md#plugins, and it’s only used in remark-validate-links, but I’m not too happy about how it works.