Add a Plugin system #237
Add a Plugin system #237
Conversation
| @@ -45,6 +45,7 @@ | |||
| "dependencies": { | |||
| "commander": "^7.2.0", | |||
| "handlebars": "^4.7.7", | |||
| "import-cwd": "^3.0.0", | |||
There was a problem hiding this comment.
we should not depend on packages from this author; they've committed to making them all ESM-only.
There was a problem hiding this comment.
eh... I don't really want to weigh in on a "don't use packages from sindresorhus" conversation regardless of what I personally think about the topic but this isn't an issue for import-cwd anyway since it's not possible to actually "import from cwd" in ESM land 🤔
There was a problem hiding this comment.
it definitely is, with dynamic import.
There was a problem hiding this comment.
so... I don't understand what is happening here. I am submitting a feature in good faith (based on a suggestion in my previous PR review) and suddenly things have gotten incredibly hostile. And I don't understand why you're brining this up @ljharb, are you a maintainer of this project or just a "concerned bystander"?
And on the point of it not being possible to "import from cwd" I get the impression that we're talking past each other here. I was implicitly saying that you can't "import from cwd" without changing the structure of the code significantly. As it turns out we're doing a dynamic require here anyway and already in an async function so it's possible to do import with a dynamic import here but that's not the point I was trying to make. If you want some more context you can follow the thread through the following links:
sindresorhus/import-cwd#3
sindresorhus/import-from#9
nodejs/node#36396
https://github.com/nodejs/loaders
There was a problem hiding this comment.
I’m bringing it up because I’m a very concerned bystander - i use this project heavily, and it affects me greatly to have it add a dependency that adds an undue maintenance burden. I’m sorry this is coming across as hostile; I’m merely reviewing a PR and providing a suggestion based on my extensive experience.
There was a problem hiding this comment.
Yes, that's right. The safer alternative i to either use a different author's package, or to write it ourselves with fs.readdir.
There was a problem hiding this comment.
@cookpete yes that's my perspective on this, it won't start failing overnight unless we update a major version of the dependency. My personal view is that this library is working as-is and we shouldn't "prematurely optimise" our approach based on a hypothetical future 🤷
But I'm more interested in getting this PR merged than having any philosophical or political debates so if you say that you would prefer me to remove this library then I'll do it 👍
There was a problem hiding this comment.
If there’s an easy replacement that makes more sense, let’s use that. If not then let’s just go with import-cwd and deal with the fallout if anything ever happens. The repo hasn‘t been touched for 3 years, and it’s been over 1.5 years since the commitment (and backlash) to convert everything to ESM. I get @ljharb’s concern but I’m also keen to not leave too much stuff floating around for too long.
There was a problem hiding this comment.
hey folks 👋 I'm still interested in getting this merged. It's been almost a year and import-cwd hasn't gone "esm only" so I guess we're safe?
Is there anything else I need to do to get this PR merged?
There was a problem hiding this comment.
We're still not safe; he plans to migrate all of them eventually, but it's fine to land it as long as we're aware that we'll want to migrate off it ASAP.
|
Finally got a chance to look through this. I like the idea – I’m just not keen on the pattern of mutating the passed in array. It feels a bit untidy? Is there a nicer way that lets the plugin return a new array instead? |
|
Ok, I had another look and I can make peace with mutating the data. I think alternatives would be more complex than it’s worth. I can also make peace with the I’ve tidied some bits up and changed the option to Nice work! |
|
omg thanks for merging this 🎉 you've made my day Any chance of a release? |
|
Hey, guys! When release is coming? |
hello again folks 👋
I know it has been a little while since I submitted #171 but I am back! and I have finally implemented a version of the plugin system that you recommended 🎉
This PR add the
--pluginoption to the cli and as you suggested it will look for a locally installed packageauto-changelog-githubif you pass--plugin githubI have implemented my previous PR as a plugin and you can see it here https://github.com/mansona/auto-changelog-github
I know we should probably add this to the documentation but I wanted to submit my first draft so that we can discuss it before finalising the design 👍
(side note: I live-streamed this implementation on twitch if you want to catch the VOD before it gets deleted in 2 weeks 😂 https://twitch.tv/real_ate )