-
Notifications
You must be signed in to change notification settings - Fork 2
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
35c3 PR #59
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thank you! I have a few preliminary thoughts, only one of which actually warrants discussion (i.e. the rest is just FND).
}, | ||
nunjucks: { | ||
plugin: "faucet-pipeline-nunjucks", | ||
bucket: "markup" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for adding this here rather than relegating it to project-specific configuration? All the other plugins are pretty much generic WRT the web pillars (admittedly Sass is an opinionated outlier, but see below), so this seems like a case of "one of these is not like the others" to me.
I'm wary of having to release a new version of faucet-core whenever we create new plugins, useful as they might be - let alone removing them again if/when we lose interest (which seems significantly more likely for Nunjucks than for Sass).
If extending plugins
from within configuration is too awkward, we should address that (e.g. require("faucet-pipeline-nunjucks/register"); module.exports = …
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should not allow arbitrary plugins into the "registry". But I would say that:
- The "pillars of the Web" is way too strict. This would also exclude Sass, TypeScript & JSX. As HTML doesn't make any sense by itself, we could only keep JS (and ESNext) plus CSS (which is not even done) here. This doesn't seem helpful. I would rephrase that as "things that are common (like SCSS, TypeScript...) and that we use regularly". This would probably make those "official" plugins.
- I would argue that the "official" plugins should also be those that are documented on faucet-pipeline.org – all "unofficial" plugins must contain their own documentation. We should however have a list of "unofficial" pipelines (we know of) on that page. It could also be argued that only the official plugins should be in the Github org.
- Using faucet-pipeline in combination with a template engine like nunjucks is an interesting use case, because it is a very simple static site generator. I would like to demonstrate that use case on faucet-pipeline.org somehow.
- To make registering the plugins less akward, I could imagine something like this in the faucet.config.js (this would require "faucet-pipeline-foo/register" which is the usual object with a plugin and bucket key):
module.exports = { foo: [{ ... }], plugins: [ "faucet-pipeline-foo" ] }
To sum it up, I agree to remove that commit. I however would like to have a system of "official" and "unofficial" plugins/pipelines and an easier way for users to register unofficial plugins.
So I think we could make this faucet-core 2.0 by
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking so long to get back into this.
As discussed, Nunjucks will be moved to a separate plugin (see #61).
I have a couple of minor comments and have tweaked a few details:
https://github.com/faucet-pipeline/faucet-pipeline-core/compare/35c3-2
@FND Your two commits look good to me, with the two adjustments discussed above. Let's adjust this and merge it into master. Then we only need #61 to make a new release. And from what I see, we are backwards compatible (except for people that use "custom pipelines"?) so we can release it as 1.3.0 – right? |
Good work 👍 Thanks! |
During 35c3, I did some hacking on faucet. This is the result: