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

Who'd like to take over this module? #25

Closed
KyleAMathews opened this issue Nov 21, 2015 · 32 comments
Closed

Who'd like to take over this module? #25

KyleAMathews opened this issue Nov 21, 2015 · 32 comments

Comments

@KyleAMathews
Copy link
Collaborator

Hi folks, I don't use this module any more and am really busy with work / other open source work and this module deserves far better maintainership than I've been giving it.

I'm tagging everyone who's contributed to this module in the past or who has an open PR. I'm willing to give out push access to everyone interested and if one of you would like to take on chief maintenance responsibility for owning the repo + NPM, I'll hand those over to you.

The main work that needs done here is to pick a solution for the array merging issue and release a 1.0.

Thanks.

@nhocki @nrf110 @anthonator @JoeStanton @nileshchavan @kodypeterson @gilbertwat @vvo @gyzerok @vyorkin @substack @SimenB @OscarGodson @rsolomo @PhUU

@OscarGodson
Copy link
Collaborator

@dadambickford and I may be able to take it over under our Yorkshire Interactive org (https://github.com/YorkshireInteractive/). We're going to release two other modules next week too and trying to do a lot more open source. Which array merge ticket were you talking about? I see 2. I can try to fix the array merge issue and if I can fix it we can take over :)

@KyleAMathews
Copy link
Collaborator Author

@OscarGodson and you're the winner! I've made you and @dadambickford collaborators so you can create the PR and merge it. Once that's in, I'll turn over NPM and transfer this repo to you. Thanks!

@OscarGodson
Copy link
Collaborator

@KyleAMathews which deepmerge array ticket needed to be taken care of?

@KyleAMathews
Copy link
Collaborator Author

#22 and #21 and #20 and #18

@InsidiousForce
Copy link

Is it dead? I was just going to use this but it seems in limbo.

@KyleAMathews
Copy link
Collaborator Author

It won't be dead if you decide to maintain it ;-)

deepmerge is a happy healthy puppy just looking for some owners.
On Fri, Mar 18, 2016 at 12:18 PM Kurt Koller notifications@github.com
wrote:

Is it dead? I was just going to use this but it seems in limbo.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25 (comment)

@InsidiousForce
Copy link

I'd need to make sure that @OscarGodson and @dadambickford lost interest, and then I have to decide if I want to take care of a puppy. It seems like a small, low-maintenance puppy.

@sohkai
Copy link
Collaborator

sohkai commented Jul 11, 2016

@KyleAMathews @InsidiousForce @OscarGodson @dadambickford Did you guys figure out anything in regards to finding a maintainer? Recently found a need to use a deep-merge function and would rather help maintain something lightweight rather than compare endlessly or build my own.

@InsidiousForce
Copy link

Sorry guys, I ended up refactoring this out of my project and am now using a few custom functions I wrote for my specific use case. @sohkai it's all yours if no one else replies.

@KyleAMathews
Copy link
Collaborator Author

KyleAMathews commented Jul 11, 2016

Another option I've been thinking about is to just deprecate this module and push people to use lodash's merge function which is very similar to this https://lodash.com/docs#merge

Lodash's team is doing a killer job and they have far more eye balls and talent working on these things. Given their recent push for modularization there's not as much point for indie functions like this.

But it's up to you @sohkai. If you want to make a strong push on this and clean things up if be happy to give you the maintainer bit here and on NPM.

@sohkai
Copy link
Collaborator

sohkai commented Jul 12, 2016

@KyleAMathews That's a good point, ever since you could pick and choose your lodash imports there's been less and less reason to maintain these standalone utilities and I guess I forgot to look through there first. I'm leaning towards seconding your thoughts about deprecation as well, seeing how similar _.merge is, although I've noticed these differences when going through the code and using _.merge on this module's tests:

  • _.merge mutates the first argument; although debatable, I see this as a plus since it's similar to the Object.assign interface that most people are aware of.
  • _.merge replaces all arrays entirely, requiring _.mergeWith and a custom array merge function to merge them; I see this as an area where this module (or another) might have an opportunity to provide useful defaults and a friendlier API. Note that this module's current implementation can be confusing in regards to Arrays with similar items merge in an unexpected manner #14 (it flip-flops between concating arrays and replacing them depending on where the array is) and notably this line causes subtle differences in regards to handling null or undefined values from sources.

I think in the end, it would be nice if there was a nice wrapper around _.mergeWith that incorporated #14's suggestions for an options parameter that supported regular use cases (but also keeping the option to pass through custom handlers to _.mergeWith), but at that point the changes would be so radical I'm not sure they belong here. At best, maybe under a 1.0 change?

@vvo
Copy link
Contributor

vvo commented Jul 12, 2016

Sorry never answered this. My 2 cents: I am only using lodash.merge theses days if I need it. And if I want to merge array values then a simple customizer function and union or concat will do it.

I would deprecate the module and ask people to use:

import {merge, union} from 'lodash';
var t = merge({}, {arr:[1]}, {arr: [2]}, function(a, b) {
  if (Array.isArray(a) && Array.isArray(b)) {
    return union(a, b);
  }
})

@KyleAMathews
Copy link
Collaborator Author

@sohkai let me know in the next couple of days if you want to take over this module and go ahead with your plan. Otherwise, let's just deprecate this on NPM and add your code @vvo to the README + point people to lodash.

Also, relevant https://twitter.com/jdalton/status/754474672977096704

@sohkai
Copy link
Collaborator

sohkai commented Jul 21, 2016

@KyleAMathews Sorry it's taking a bit for me to get back to you about this; I'm planning to have a look through some of the other options to double check how they've been built but in the event none of them just wrap _.merge, I'll take this over and go with what I've mentioned for a 1.0 release.

@sohkai
Copy link
Collaborator

sohkai commented Jul 21, 2016

Had a chance to look through a number of other repos and it seems like they've all chosen to re-implement everything themselves without providing as much flexibility. Sign me up for the work!

@KyleAMathews
Copy link
Collaborator Author

Ok! Awesome! Thanks! Down with unmaintained code! I just added you as a contributor here. Once you're ready to make a new NPM release, I'll figure out how to make you a contributor there :-)

@kasparsklavins
Copy link

kasparsklavins commented Jul 28, 2016

Or alternatively, mark it as a final release.

@TehShrike
Copy link
Owner

Sure, I'd be willing to maintain.

I maintain a good number of small modules like so:

  1. if there's a test and the pull request looks good, I publish right away after merging. I don't like leaving unpublished code in master.
  2. I don't have a bower account or anything, so I don't publish to there

If that sounds good, then feel free to add me as a contributor on Github and npm.

@KyleAMathews
Copy link
Collaborator Author

@TehShrike awesome! Added here on Github — what's your NPM username?

@TehShrike
Copy link
Owner

I am tehshrike on npm.

@KyleAMathews
Copy link
Collaborator Author

Added you as owner on NPM.

@TehShrike
Copy link
Owner

By the way, if anyone's curious, I'm a big fan of the open governance style Rod Vagg describes in this episode of the Changelog podcast.

@KyleAMathews
Copy link
Collaborator Author

Me too :-)

@macdja38
Copy link
Collaborator

I would definitely love to help out maintaining this module, I've found it extremely useful and I want to make sure it remains around for as long as possible.

@KyleAMathews
Copy link
Collaborator Author

@macdja38 great Jacob! Added you as a collaborator on Github? What's your NPM username? I'll add you as an owner there as well.

@macdja38
Copy link
Collaborator

@KyleAMathews same as here macdja38

@sohkai
Copy link
Collaborator

sohkai commented Sep 27, 2016

@TehShrike @macdja38 Thanks for picking up this as well! My original intent was to sprint towards the changes I proposed in #25 (comment) and close PRs/issues after, but obviously this hasn't happened yet 😣.

With #37, and a lot of good work already done by @TehShrike, my proposed probably don't make sense anymore, since we've got maintainers who would like to keep the current behaviour as-is 😄.

(And thanks @KyleAMathews for picking up over my negligence ;))

@TehShrike
Copy link
Owner

TehShrike commented Sep 27, 2016

Installing the lodash.merge package gives you a 56KiB JS file to import. I'm fine maintaining this module!

@TehShrike
Copy link
Owner

@KyleAMathews would you be willing to head over to https://travis-ci.org/profile/KyleAMathews and toggle this repo on? It looks like since it's a public non-org repo in your name, you'll have to be the one to enable it. I think that should be all you have to do going forward, though.

@KyleAMathews
Copy link
Collaborator Author

@macdja38 added you as owner
@TehShrike activated deepmerge on travis.ci

@TehShrike
Copy link
Owner

Closing for now, but if anyone else wants to contribute, post here or ping me anywhere else!

@KyleAMathews
Copy link
Collaborator Author

👏👏👏 great work @TehShrike!

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

8 participants