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

v2.5.0 breaks integration with metalsmith-watch #183

Closed
exortech opened this issue May 11, 2022 · 6 comments
Closed

v2.5.0 breaks integration with metalsmith-watch #183

exortech opened this issue May 11, 2022 · 6 comments

Comments

@exortech
Copy link
Contributor

exortech commented May 11, 2022

The change made in this commit from using multimatch directly to using metalsmith.match() breaks the integration with metalsmith-watch. The call to metalsmith.match() returns an empty array, which (correctly) triggers the "no files to process" error. Whereas matching against files directly correctly populates the matchedFiles variable.

@exortech
Copy link
Contributor Author

The issue is that layouts is not passing the files collection through to metalsmith.match. As a result, metalsmith.match is using the _files member variable to perform the match on - rather than the transformed file collection that is passed into the plugin chain. This creates a failure if any of the files have been renamed during the build process.

@exortech
Copy link
Contributor Author

Thanks @webketje . I can confirm that change works as well.

One consideration though is that it does create a dependency between upgrading metalsmith and upgrading metalsmith-layouts. If only one is upgraded then the issue will remain.

On the surface, it seems like this is a "regression" in metalsmith-layouts. And so it seems like the fix would be there (especially with this being a minor version change).

Anyway, ultimately I'm just happy to see this issue getting fixed and will go with whichever course of action you decide to take. 😀

@webketje
Copy link
Member

webketje commented May 14, 2022

Thx a lot for your thoughts, as community feedback is hard to get by for MS. Feel free to join discussions in the Gitter chat anytime!
What follows is documentation on my stance for future developments (I might post a news item about it on the website)

One consideration though is that it does create a dependency between upgrading metalsmith and upgrading metalsmith-layouts. If only one is upgraded then the issue will remain.

Plugins are still 'pure' with the expectation that the metalsmith parameter provide a compliant interface (and match method).
The decision to include plugin utility functions in core (.env, .debug, .match) and specify metalsmith as a peer dependency does make the plugins less standalone, but it is a necessary trade-off to avoid having conflicting dependencies (multi, micro, mini, nanomatch), and different dependency versions (ie to have a unit-tested standard). It may help improve metalsmith's popularity as it is really barebones compared to competition (like 11ty). For debug for example, it is impossible to have a single log file if every plugin includes its own debug instance. Ditto for internal state: I might need to include it in the future to make developing sites with metalsmith more convenient.

I wanted to address one more comment on the PR: #184

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.

In the past metalsmith has often been held back by backward-compatibility, with issues resulting in no action being taken at all.
Plugins like metalsmith-debug-ui (which I find really valuable) and metalsmith-watch (which you find really valuable) do some unorthodox things like accessing private MS members, adding plugin-specific patches and trying to hook into the build sequence by piecing it together themselves. If a plugin needs to significantly alter the way metalsmith processes files, then it is not a plugin but rather a utility "on top of" metalsmith (debug-ui takes the more correct approach of requiring to "patch" the metalsmith instance). Browsersync & dev-servers fall in this category as well. Partial reload is extremely valuable and indeed not possible at the moment without metalsmith-watch, but ultimately this should either be provided by either core or a non-plugin utility. So I will maintain compat for MS 2.x but will not guarantee it for MS 3.x

@exortech
Copy link
Contributor Author

The decision to consolidate utility functions in core seems to me to be a good thing. It creates consistency and reduces the dependencies that need to be managed by the plugins.

As for internal state, I'm less convinced about that. I'm not sure what internal state needs to be maintained by metalsmith core. However, in the case of the _files collection, that still feels misplaced to me. I don't understand why the state wouldn't just be passed through the plugin chain.

I haven't used metalsmith-debug-ui as I haven't done a lot of ms plugin development. One plugin that I have put a bit of work into extending is https://github.com/doteco/metalsmith-i18next. The original project is unmaintained and the author's github account is inactive. I'd be willing to take over this plugin as I think supporting multi-lingual static site generation is a useful thing.

Regarding metalsmith-watch, I think that this type of functionality (file watching, incremental builds, livereload) is hugely valuable. It is available out-of-the-box with 11ty. I did look at just using chokidar; however, there would be no default support for incremental builds (essential for larger sites) or livereload.

In terms of a path forward for metalsmith-watch, it could certainly be modernized with a bit of work - there's not really that much code there. There are a number of unprocessed PRs, including one switching from gaze to chokidar, that could be processed. Dependencies could fairly easily be brought up-to-date. The collections code could be cleaned up.

Given that it could be considered key or common functionality for metalsmith, what's the process for bringing plugins under the metalsmith org?

@webketje
Copy link
Member

As I will merge this PR & close this issue & this discussion is out of scope of the current repo, I've invited you to a GH team: https://github.com/orgs/metalsmith/teams/ideators. This space has posts about core plugin guidelines AND ideas for a multilang plugin (which I will also need soon professionally). Core plugins should conform to the structure at: https://github.com/metalsmith/core-plugin. I've created a post here regarding ms watch/devserver etc: https://github.com/orgs/metalsmith/teams/ideators/discussions/5

Given that it could be considered key or common functionality for metalsmith, what's the process for bringing plugins under the metalsmith org?

In the post I linked I explain why I don't see this as core functionality, however I would consider adding it if there is just no other way to do it cleanly. We have adopted some third-party plugins, but basically I have to be convinced to bring it under the metalsmith org (for watch I am not at the moment)

webketje added a commit that referenced this issue May 15, 2022
Resolves #183: pass the transformed list of files to metalsmith.match…
@exortech
Copy link
Contributor Author

Thanks for merging. What's the process for releasing a patch with this merge? I'd like to move off of my fork back to the main repo.

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