-
Notifications
You must be signed in to change notification settings - Fork 2
v3.0.0 #147
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
Conversation
CHANGELOG.md
Outdated
|
|
||
| notable changes for end users: | ||
|
|
||
| * dropped support for obsolete Node versions; now requires Node v18 or later |
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.
🤔 Is there a more concise (yet elegant) way to express this?
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.
Yes, there is:
requires Node v18 or later
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 had considered that, but wanted to be explicit about dropping support. Perhaps:
bumped Node requirement to v18 or later, dropping support for obsolete versions
🤷
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.
Okay, works for me 👍 (I would say that "requires version xy" in a changelog implies that this drops support for older versions.)
| notable changes for end users: | ||
|
|
||
| * dropped support for obsolete Node versions; now requires Node v18 or later | ||
| * reduced number of dependencies |
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 sounds far too mundane: It's a significant improvement IMO, but ... 🤷
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 would keep it. Most people won't care, but I think it is still worth putting four words in the changelog.
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.
My concern was more that this sounds like a boring line item when it's really "yay, look at what @moonglum figured out!"...
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.
Thanks, appreciated 🙇
| notable changes for end users: | ||
|
|
||
| * dropped support for obsolete Node versions; now requires Node v18 or later | ||
| * reduced number of dependencies |
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.
Also, I opted not to list "updated dependencies" as a separate point here; seems implied?
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.
Fine with me 👍
CHANGELOG.md
Outdated
| * switched to named instead of default exports | ||
|
|
||
| this might affect users of utility functions like `createFile` or | ||
| `resolvePath` |
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'm not sure whether this is even worth mentioning, but it also doesn't hurt? (See de0a193 for details.)
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.
Will this break any of the official plugins? 😓
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 didn't think so, but it appears I'd overlooked that both faucet-static and faucet-images use FileFinder... 😬
Relatedly, I recently realized that some projects (including Node itself) use lib/internal to distinguish exports. Not ideal (and a bit enterprisey), but the only realistic alternative seems resorting to a _ prefix for such cases...
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'm a bit scared of this change. Maybe we should skip it for a patch version 🙈
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.
Unfortunately, reverting that commit isn't easy.
I've got a patch that might restore backwards compatibility with stuff like this:
let exports = module.exports = function() {
console.error("WARNING: default exports are deprecated; please switch to named exports");
return exports.createFile.apply(this, arguments);
};
exports.createFile = function createFile(filepath, contents) { /* … */ }However, turns out that ae3f548 also introduced breaking changes for parseCLI, readConfig, pluginsByBucket, loadExtension, static and live by turning them async. Are we worried about that?
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.
instead, that might be a pragmatic solution for ... which problem again?
I remember we had some really frustrating experiences for people because as soon as npm decides to install two versions of faucet core, everything breaks. And I mean EVERYTHING 😂 This left me paranoid, especially because I can't remember what exactly causes this to happen 🙈
I'd rather have faucet-images depend on faucet-static instead
This is actually a valid point. FileFinder could move into static, and images could depend on that 🤔 images is basically a copy of static, with some processing sprinkled on top. So it would be a chance for some deduplication. This would not only be for aesthetics: There is a bug in static (faucet-pipeline/faucet-pipeline-static#24) which is also in images, as they share the same code. So if it was fixed in static, it would automatically be fixed in images as well 🤔 Is it okay for you if I try that out before we do the release? Then all packages except aiur (which is < 1) would be compatible with 2 and 3.
That release plan seems correct to me. 👍
Great! 👍 Yes, let's run npm outdated for each of the packages before we release it, and go to node 20 as the minimal version.
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.
Is it okay for you if I try that out before we do the release?
Sure, knock yourself out. You might want to consider #148 while you're at it - otherwise just reject that.
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 just noticed that your release plan omitted nunjucks, which also continues to support faucet-core v2.
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.
moving this discussion to #149
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.
OK boomer
No description provided.