search

metalsmith / layouts

A metalsmith plugin for layouts
MIT License
116 stars 49 forks source link

Resolves #183: pass the transformed list of files to metalsmith.match… #184

Closed exortech closed 2 years ago

exortech commented 2 years ago

… so that renamed files can be matched

https://github.com/metalsmith/layouts/issues/183

webketje commented 2 years ago

@exortech thx, can you give me a short step-by-step instruction to reproduce the issue (or a GH repo or repl.it). The 'cached' copy of metalsmith files that metalsmith.match uses when you omit the second param, should be a reference to one and the same object. So even when file keys change, they should still be matched.

If this only occurs on repeat runs using metalsmith-watch that plugin probably does something unexpected with the files object (like, cloning it). FWIW I agree with comments from previous metalsmith owners & maintainers in previous ms-watch issues (don't remember which ones in which repo by heart) that watching should rather be done with a separate tool like https://www.npmjs.com/package/chokidar instead of as an ms plugin.

That's not an excuse, and this PR is really good to go, but before merging I would like to understand the exact cause.

Update just had a look at metalsmith-watch source: it looks unmaintained, with outdated deps and likely to break as it contains a lot of old fixes for ms-collections. And I confirmed my suspicion that it re-used a metalsmith instance to run on a different files object. However I may port this issue to the main metalsmith codebase and release a bugfix there to re-set files on every run.

exortech commented 2 years ago

@webketje this is the repo for one of the static sites we build with Metalsmith that was having the issue: https://github.com/doteco/www. If you check out build.js you can see the list of plugins used in the build.

It's a bit complicated to produce a simple repro; however, the gist is that, as you noted, metalsmith-watch creates a new file list (containing only the modified files to rebuild) and then invokes metalsmith.run.

I'm not that familiar with the metalsmith codebase, so I can't speak to the best way to fix it. However, while perhaps unmaintained, metalsmith-watch is a really useful plugin. And I imagine that there are potentially other plugins out there that could be doing something similar. To prevent those plugins from breaking, it may be best to preserve the original behaviour - especially as it is a very simple change to ensure that the file list is passed it. It's behaviour that's already supported by the existing interface.

Also, it seems like there's no reason why metalsmith-layouts should assume that the original files list is what is being passed to the plugin. I'm unclear why it's necessary for metalsmith to maintain this state instead of just operating as an immutable, pure function.

webketje commented 2 years ago

[...] it may be best to preserve the original behaviour - especially as it is a very simple change to ensure that the file list is passed it. It's behaviour that's already supported by the existing interface. Also, it seems like there's no reason why metalsmith-layouts should assume that the original files list is what is being passed to the plugin.

I totally agree with you on this.

I'm unclear why it's necessary for metalsmith to maintain this state instead of just operating as an immutable, pure function.

It is not necessary. The _files "cache" was a quick solution to provide a super-easy signature for metalsmith.match but the mechanism needs to be made more bullet-proof. The run function should indeed act as an immutable pure function.

webketje commented 2 years ago

@exortech Could you help validating an alternative patch to Metalsmith itself works by temporarily replacing node_modules/metalsmith/lib/index.js contents with those in the hotfix branch I just pushed here: https://github.com/metalsmith/metalsmith/blob/hotfix/ms-match/lib/index.js and then do some repeat runs?