Skip to content
This repository was archived by the owner on Jul 16, 2020. It is now read-only.

updating, reducing dependencies#9

Closed
trevorhreed wants to merge 1 commit intobusterjs:masterfrom
trevorhreed:master
Closed

updating, reducing dependencies#9
trevorhreed wants to merge 1 commit intobusterjs:masterfrom
trevorhreed:master

Conversation

@trevorhreed
Copy link

The version of lodash being used in the published version of the multi-glob npm package has a serious security vulnerability. This pull request updates that dependency. In addition, instead of requiring the entire lodash library, this update requires only those portions of lodash that are necessary.

@mroderick
Copy link
Contributor

Did you try running the tests locally?

I think there needs to be a bit of maintenance done to the repository, before this can be made to pass. Would you be up for doing that also?

@trevorhreed
Copy link
Author

trevorhreed commented Oct 31, 2018

Possibly. It appears that travis-ci and appveyor are running things against old versions of node, but I think it might be running a newer version of npm than is supported. The scripts/configuration on travis/appveyor may need updating as well. What's the minimum version of node that you would like to continue supporting?

@mroderick
Copy link
Contributor

A bit of background, the main project under this GitHub organisation: buster has been discontinued. Some repositories have been moved to the sinonjs organisation (formatio, referee, samsam), where work continues.

However, none of the sinonjs projects use multi-glob, so I'd be hesitant in adopting that one into sinonjs, or continue to do maintenance on it.

Would love to hear from some of the other @busterjs/owners, perhaps @dominykas has an opinion on whether or not to continue maintenance on the busterjs repositories.

Do you use multi-glob directly yourself, or is it used in one of your dependencies?

What's the minimum version of node that you would like to continue supporting?

Just the LTS versions, so that would be 6 (until April 2019)

@jodal
Copy link
Member

jodal commented Nov 1, 2018

According to https://www.npmjs.com/package/multi-glob?activeTab=dependents this project has 50 public dependents.

@trevorhreed
Copy link
Author

I have recently found a suitable alternative (globs). However, since this package still has quite a bit of usage, I thought I could at least try to help get the lodash dependency updated. Perhaps it would be best to deprecate in favor of globs?

@mroderick
Copy link
Contributor

Perhaps @stephenmathieson or @thomas-darling could comment on whether or not they are planning to maintain globs. It would be silly to deprecate this module in favour of another module that is then deprecated.

@stephenmathieson
Copy link

I do not have intentions of maintaining the globs package, and handed ownership over to @thomas-darling.

@Natim
Copy link

Natim commented Dec 10, 2018

Apparently the travis config needs to be updated as well.

@Natim Natim mentioned this pull request Dec 10, 2018
@dominykas
Copy link
Member

According to https://www.npmjs.com/package/multi-glob?activeTab=dependents this project has 50 public dependents.

But it also has only 18k weekly downloads and it hasn't been updated in 3 years and most of the packages that depend on this have also not been updated in over a year, so I wonder if this should just be marked as deprecated?

And the updates that it had also have not changed the main file - which is now 6 years old... The code itself is less than 50 lines of code - easy enough to fork and publish, whereas maintaining this would mean upgrading the node versions, changing the test framework and doing quite some cleanup.

I'm not sure it's worth it. At the same time, given last month's fun with event-stream, I'm not sure that handing over to a new maintainer is best practice either, so I'm saying that someone should fork this, if they want to, and we can add a link to the new package and alternatives in the readme and the deprecation notice?

@dominykas
Copy link
Member

In fact, I think all of busterjs organization should be marked as archived, and all its packages should be marked as deprecated. @mroderick what do you think? I can probably find time for that this year. It's been a good run - it's time to retire, there's plenty of alternatives.

@augustl
Copy link
Member

augustl commented Dec 11, 2018

I don't disagree fwiw! We started Buster when the only alternatives were QUnit and jsTestDriver, but there are plenty of great options out there nowadays. And a valuable lesson of shipping sooner and building iterative traction, rather than spending time on perfecting stuff and not releasing it, was learned by yours truly :)

@cjohansen
Copy link
Member

cjohansen commented Dec 11, 2018

I agree that archiving these repos would be the right thing to do given that they are truly abandoned (except for the modules expertly curated by the sinonjs crew under @mroderick's leadership).

Still, I think removing a module that have 50 dependents and "just" 18k weekly downloads is a bad idea. Our industry suffers way too much breakage already, let's not contribute to the bonfire. I made a pass at fixing the issues raised in this PR over here: #11 If someone with the keys to the castle could be bothered to merge and release to npm, then 👍

@cjohansen cjohansen closed this Dec 11, 2018
@dominykas
Copy link
Member

Just to make sure - I wasn't suggesting removal, only adding a deprecation notice that this is no longer maintained.

I have to 🙇 down to @cjohansen for taking the time to do a cleanup PR, though - I'll merge and publish.

@mroderick
Copy link
Contributor

mroderick commented Dec 11, 2018 via email

@cjohansen
Copy link
Member

Awesome @mroderick! Even though it isn't "actively maintained" - it really is done, and now it has running tests. No point in keep poking at it. If people use it, let 'em.

@dominykas
Copy link
Member

dominykas commented Dec 11, 2018

@mroderick shall I add you as an owner on npm?

@mroderick
Copy link
Contributor

@mroderick shall I add you as an owner on npm?

yes please

@mroderick
Copy link
Contributor

We should probably also transfer bane, as it is used by @sinonjs/referee (which we're actively working on)

@mroderick
Copy link
Contributor

Perhaps we can get a couple of maintainers to go with the new packages? ;)

@cjohansen
Copy link
Member

Rather than dragging bane along, couldn't referee just use EventEmitter?

@mroderick
Copy link
Contributor

Rather than dragging bane along, couldn't referee just use EventEmitter?

We're still trying to keep referee in a state where it can run as-is in browsers, down to IE11.

@mroderick
Copy link
Contributor

In any case, I think it would be a good idea to start marking the unmaintained repositories under busterjs/ as such and archive them. I am happy to take on multi-glob, we can even just put it in it's own organisation.

@dominykas
Copy link
Member

@mroderick added you on bane and multi-glob in npm.

We don't strictly need to transfer bane or multi-glob until a good home comes along?

I'll go through the remaining repos and archive them on GH and deprecate on npm at some point later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants