Skip to content
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

metalsmith.match() issue with metalsmith-branch #24

Open
emmercm opened this issue Jul 29, 2022 · 4 comments
Open

metalsmith.match() issue with metalsmith-branch #24

emmercm opened this issue Jul 29, 2022 · 4 comments

Comments

@emmercm
Copy link

emmercm commented Jul 29, 2022

Something I've noticed in my own plugins @webketje is how metalsmith-branch will only pass a subset of files to a child plugin, but if you use metalsmith.match() it will match against the "global" list of files, and you will end up with some filenames that might not be in your "working" file list.

The fix is using metalsmith.match(pattern, Object.keys(files)) in every plugin, like this: https://github.com/emmercm/metalsmith-reading-time/pull/88/files#diff-92bbac9a308cd5fcf9db165841f2d90ce981baddcb2b1e26cfff170929af3bd1R16

@emmercm
Copy link
Author

emmercm commented Jul 29, 2022

The most recent plugin update here is causing my function parameter to be undefined for files that are matched with metalsmith.match() but aren't in my branch:

https://github.com/emmercm/www/blob/e285f05d09eaf33d32520181a1b1a5bfe607a3ad/index.js#L491

@webketje
Copy link
Member

webketje commented Jul 29, 2022

A similar issue has already been reported at metalsmith/layouts#183; there I expressed some elaborate points of view concerning plugins like branch/watch. Not sure why you reported on this plugin rather than metalsmith itself?

TBH I'm not sure how to "resolve" this as it is intended behavior. metalsmith.match works as documented at https://metalsmith.io/api/#Metalsmith+match and metalsmith itself has no notion of a "temporarily altered filelist" (and I'm keen to keep it out). If plugins author wish, they can go this route (but rather by using metalsmith methods like I did for a local plugin here: https://github.com/metalsmith/js-bundle/blob/main/test/index.cjs#L22, than by doing 'unorthodox things' like requiring MS private dependencies).

The broader issue title is "how to run a plugin sequence on a subset of files?", to which I would say specific plugins should provide a pattern option or target only relevant files (eg permalinks does .html only)

The metalsmith-branch plugin has several issues:

  1. it is not CLI-compatible
  2. it reconstructs ms internal logic using ware
  3. it is incompatible with any plugin making changes to the Files object keys (=file paths)

Below would be a more "compliant" branch plugin (untested, no support for nested branches).
Note that when using metalsmith.run instead of ware.run, metalsmith.match shorthand will target only the relevant files, as the _files cache is reset within this method

function configureBranch(pattern, ...plugins) {
  return function branch(files, ms, done) {
    const subset = ms.match(pattern, Object.keys(files))
      .reduce((subset, key) => {
      subset[key] = files[key]
      subset[key].__originalPath = key
      return subset
    }, {})

    ms.run(subset, plugins)
      .then(subset => {
        Object.entries(subset).forEach(([path, file]) => {
          files[path] = file
          if (file.__originalPath !== path) {
            delete files[file.__originalPath]
          }
          delete file.__originalPath
        })
        done()
      })
      .catch(done)
  }
}

// usage
ms.use(branch('**/*.md',
   layouts(), // these plugins can safely use metalsmith.match(pattern) shorthand
   markdown()
))

@emmercm
Copy link
Author

emmercm commented Jul 29, 2022

I opened an issue on this plugin specifically because it's the one that is causing the problem for me. But as I admitted, the "fix" I've been putting in my plugins is required for every plugin in order to work around metalsmith-branch. I didn't realize there was an issue with the manipulation of the files object.

I think it's somewhat unrealistic to expect every plugin to have a pattern option, even if that would be ideal. We can probably count on one hand the number of active plugin developers, or ones that even respond on GitHub. You can see how many issues I've opened on abandoned plugins over the last 3 years: https://github.com/issues?q=is%3Aopen+is%3Aissue+author%3Aemmercm+archived%3Afalse+metalsmith

Your issue looks fairly elegant, is that something one of us should create, or how do you feel about it being a core feature of Metalsmith?

@webketje
Copy link
Member

webketje commented Aug 1, 2022

Ok in that case, what I can do to resolve this issue is:

  • 1. make sure the core plugins use the match(pattern, files) signature
  • 2. write a note in the API docs about the potential pitfalls when using branch/watch

I feel this shouldn't be a core feature of metalsmith, cf:

metalsmith itself has no notion of a "temporarily altered filelist" (and I'm keen to keep it out)

MS takes a source and a destination and that's it. Another way to solve it would be running multiple ms builds using ignore and clean(false) or different source folders.

But feel free to use my snippet as a starting point for a new branch plugin.
I'll mark this issue closed when I've gotten to the 2 points above

@webketje webketje reopened this Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants