Skip to content
This repository has been archived by the owner on Oct 2, 2019. It is now read-only.

Is this project still being maintained? #1108

Closed
cdjackson opened this issue Jul 25, 2015 · 32 comments
Closed

Is this project still being maintained? #1108

cdjackson opened this issue Jul 25, 2015 · 32 comments

Comments

@cdjackson
Copy link
Contributor

Activity from the maintainers seems very low and there are hundreds of PRs and issues.

Is this project still being maintained, or can someone suggest an alternative?

@wald-tq
Copy link

wald-tq commented Jul 27, 2015

I think this project is the best bet for a angular native implementation of a select2 field.

There have to be many developers which are using this directive and they know the way around the hard edges (including me).

Another alternative could be Angular Material with the nice chips: https://material.angularjs.org/HEAD/#/demo/material.components.chips

@cdjackson
Copy link
Contributor Author

Thanks. I agree - this is still a good project, I'm just concerned that it's not really being maintained and therefor it will quite soon become not such a good project :(

There have to be many developers which are using this directive and they know the way around the hard edges (including me).

This is what concerns me :) I'm also making some modifications and have submitted a few PRs over the past few months, but there are now around 150 open PRs, and 470 or so open issues. It would be great if the maintainers no longer have the time to support it if others were able to...

The longer PRs are left unprocessed, the more difficult it is to merge them as there will be conflicts...

@wald-tq
Copy link

wald-tq commented Jul 27, 2015

I'm also a bit concerned. I use it throughout my project and I don't have a better choice for now.

Also see this discussion: #1096

@ProLoser
Copy link
Member

Are either of you interested in taking this project over?

@sars
Copy link

sars commented Jul 27, 2015

@ProLoser , I'm interested also in development this project. Ready to help

@dimirc
Copy link
Contributor

dimirc commented Jul 28, 2015

@sars one way to help could be to try checking duplicated issues and posible PRs

@cdjackson
Copy link
Contributor Author

Are either of you interested in taking this project over?

I would be happy to help with the project. I think since this is such a popular project that it needs a few people to help maintain it as it needs to be regularly maintained. Currently, we go for months without any activity from the maintainers, and hence we end up with a totally unmanageable number of PRs and Issues... I don't mean to criticise, people have a lot of demands on time these days, but the project can't thrive if nothing changes for months on end...

I'm not currently hugely familiar with the source, but would be willing to help maintain it. I would be happy to initially spend a few days going over the issues found in #1096. From there I would suggest it might be easier to close "all" old issues (say, older than 4 months) - or at least request that it is still valid. Likewise for PRs - many already have conflicts and need to be updated against the last codebase. This could allow the project to be brought back into a manageable situation where a few people should be able to keep on top of things every day or three (and I would be happy to support this too)...

Just my 2p :) Maybe the above suggestion is a little drastic, but this is a nice project, and I think it needs something to bring it up to date...

@ProLoser
Copy link
Member

@cdjackson That's fine.

@dimirc
Copy link
Contributor

dimirc commented Jul 28, 2015

@cdjackson agree also

@cdjackson
Copy link
Contributor Author

Just an update on what I'm doing. I've removed a lot of watches which i don't think are necessary, and are probably bogging down the digest cycle. I've completely removed tagging and refresh from the core and have added lifecycle handlers (currently onKeypress, onBeforeRemove, onBeforeSelect and onBeforeDropdown) which should allow tagging and refresh to be implemented externally - or various other forms of the control.

The reason I picked on tagging as a starting point is it is spread throughout the control and it made the code a bit messy (IMHO!). Possibly the onKeypress() callback doesn't allow as rich a tagging implementation (I've not spent too much time testing this), but it is ok, and these callbacks ought to allow better implementations than my quick test tagging implementation. I've also used this to create a combo-box control, which allows you to select from the list, or type in whatever you like, and this ought to be possible with these callbacks.

I've also updated the build to produce separate implementations for the different templates (the templates account for ~10k), and separated out the sort directive... It's still work in progress, but thought I'd mention what I was doing... If it's outside of what is required for this repo, then at least I'll be in a position to make my combo-box control :)

A rough estimate of size reduction (comparing to minified versions)-:
Original ~35k
Remove tagging ~31k
Remove refresh ~30k
Remove templates ~25k with just a single theme

The sort directive is about 1k as well... I've also fixed a few bugs along the way...

This is clearly a significant (and breaking!) change to the current repo (well, if you are using tagging or refresh anyway!), so I'm not sure if it's of interest or not. If it is, I'm happy to tidy, sort out the tests and provide it for review - otherwise I'll just stick with this in my projects :)

@ProLoser
Copy link
Member

ProLoser commented Aug 5, 2015

You should open a pull request so we can start providing feedback (although I expect it to be nearly unrecognizable lol). Alternatively, perhaps we should give you push rights and let you work on a branch on this repo so that we can keep this a collaborative effort and other people might be poised to support your endeavors.

One thing I'd like to see is that we still (to some degree) implement some of the most critical prior features, but doing so in this composeable approach. For instance, demoing how tagging or custom entries would be done with this new API will help other devs further rip out core code.

@ProLoser
Copy link
Member

ProLoser commented Aug 5, 2015

What is refresh btw? And what sorts of watchers have been removed? If these are things that can be converted to JIT (Just In Time) evaluations, that's always better. It's not like you need to keep the settings up to date while you are NOT interacting with it, and it's not like it's a reasonable expectation for the widget to update while you ARE in the middle of interacting (or if this is the case, allow people to do this themselves).

@cdjackson
Copy link
Contributor Author

although I expect it to be nearly unrecognizable

Maybe not unrecognizable, but certainly a diff will tell you (I think) that every line has changed as I've reformatted, added more comments, and fixed a few bits of code...

perhaps we should give you push rights and let you work on a branch

That's also fine - I'm not sure that you can limit access to a branch, but I can play nicely :)

One thing I'd like to see is that we still (to some degree) implement some of the most critical prior features

I agree - I have re-implemented tagging (in a basic sense!) for exactly this reason (and I intend to use it!). Currently, it probably doesn't support a bunch of features, but using the onKeypress, it should be possible to do most/all of what the current binding does. For example, tagging works fine (from my basic tests) but I only support enter to end tagging, not a list of tokens, but this should be easy to implement.

Additionally, I have the combo style, which uses the single select to add custom entries - maybe the control already did this, but I couldn't see how and the docs weren't forthcoming...

Both of these are working in a demo form (bootstrap only) that's included in my test branch (newdemo.html/js).

What is refresh btw?

It is/was an async loading callback. I didn't look at the implementation in detail, but it called a user specified method when the control was opened, and then called it again every user defined seconds. I've not tried to implement this in my demo, but it shouldn't be hard - I've added a callback for the dropdown opening and closing, so the user can start/stop a timer to do whatever async stuff they like!

And what sorts of watchers have been removed

Many of the watches/observes on configuration options - it's possible I might have gone too far, but things like searchEnabled, sortable, all the tagging options (tokens, label etc) were watched but these have been removed now anyway. Relevant options like the searchEnabled could be added back into a method that re-evaluates on dropdown if someone really thinks they need to be changed dynamically - this would be simple... I'm sure someone will tell me there's a use case, but I was in a slightly ruthless mood :)

The way things are at the moment is I've not added extra API methods for manipulating the control within the callbacks. I'm split between adding specific public API methods and potentially reducing flexibility, or just allowing interaction directly with the controller. I'm happy to take 'angular-ui' standard practice/guidance if there is any on how you'd prefer to see this...

Currently my mods are in https://github.com/cdjackson/ui-select/tree/refactor_branch

@ProLoser
Copy link
Member

ProLoser commented Aug 7, 2015

@cdjackson will need some place to provide feedback on your branch.

Few notes, I was poking around your addons, you are not making of this controller methods. The first and foremost priority is to expose EVERYTHING as a controller method. This allows you to inject the directive-controller and override the methods (making onWhatever callbacks a redundant convenience tool).

I don't remember what the lookup MAP is for, but I don't really know if we should bother with it anymore. If we use the mapped keys explicitly in our code, that's fine, but if we're trying to expose some sort of convenience to users, it should be removed. Let devs sort out keycode aliasing themselves.

Refresh is stupid and should be removed. Instead, simplify and expose the collection of items we use to render the dropdown. This will allow users to hack this list at any time they want with whatever they want. This feature definitely does not belong in the core.

Again, please RELIGIOUSLY pore over this new directive I assembled (and the README examples) for guidelines on what I'd like to see: https://github.com/angular-ui/ui-mention/blob/master/src/mention.es6.js

I agree with removing all these silly watchers. We can simply set boolean flags on the uiSelect controller that are checked whenever the control is interacted with.

@cdjackson
Copy link
Contributor Author

The first and foremost priority is to expose EVERYTHING as a controller method. This allows you to inject the directive-controller and override the methods (making onWhatever callbacks a redundant convenience tool).

Yep - that's what I thought you might say, and that's fine. Would you keep the callbacks for application level calls?

I don't remember what the lookup MAP is for, but I don't really know if we should bother with it anymore.

It's used to detect key changes - I've already deleted most of it and just kept the special keys which are used (that removed a couple of kb).

Refresh is stupid and should be removed.

Yep - I deleted it :) The same functionality should be possible using onDropdown (etc) callbacks.

@ProLoser
Copy link
Member

ProLoser commented Aug 7, 2015

@cdjackson so right now the issue with stuff like onBeforeSelect (which I would just call beforeSelect) and onAfterSelect and other such callbacks is that they are still us explicitly exposing API, which although is a step in the right direction, is not exactly what I had in mind for this iteration.

Instead, just do onSelect which contains OUR code. Essentially you're not exposing optional callbacks just so the user can hook into them, which means we are trying to plan for and accommodate for the user. Instead, we should be exposing what we are and need to do anyways in a way that can be overridden.

For instance:

// public api: $select.onBeforeSelect()
// public api: $select.onAfterSelect()

Which is called from private api
// private api:
onSelect(choice) {
  // do prep
  if ($select.onBefore) $select.onBefore()
  // do swap
  if ($select.onAfter) $select.onAfter()
  // do cleanup
}

The issue with this approach ^ is we are still hiding code from being modified, and what we publicly exposed might eventually not be enough if the user so chooses, or we have to keep exposing more and more parameters to these callbacks, etc.

This is not designed with refactoring-friendly mentality.

Instead, watch how we can lump all of this together into a simpler api that gives devs more control:

$select.onSelect(choice) {
  // do prep
  // do swap
  // do cleanup
}

// The dev can then override this mandatory method which actually contains logic (and not just stubs) to yield the same results as above:
var onSelect = $select.onSelect;
$select.onSelect = function(choice) {
  // do custom prep (beforeSelect)
  onSelect.call($select, choice);
  // or
  myCustomSelect.call($select, choice);
  // do custom cleanup (afterSelect)
}

We have now given devs the ability to add before, after or just completely replace the select behavior completely if they so choose. There is no 'optional' apis that we tried to 'guess' at, nor do we have to come up with stateful parameters to provide to these callbacks or worry about them messing up the state in any way that wasn't already expected.

So again, write EVERY SINGLE PIECE OF LOGIC that we use as a publicly replaceable/accessible method. This gives people the ability to monkey patch and extend behavior.

In addition, try to shrink down methods as much as possible into implementation-agnostic methods.

This means onSelect() should essentially do nothing more than call a series of more concise methods like so:

$select.onSelect(choice) {
  this.setChoice(choice);
  this.render();
  this.close();
}

By breaking this up into smaller, yet still publicly exposed methods, we allow devs to be less wary about completely modifying or replacing behavior. They don't have to copy-paste a 40-line function just to tweak 1 line.

@jnelson
Copy link

jnelson commented Aug 8, 2015

@ProLoser 👍 This is pretty.

@ProLoser
Copy link
Member

ProLoser commented Aug 8, 2015

This is just object oriented methodologies.

@awerlang
Copy link
Contributor

While I agree on most of the proposed directions, this specific point:

So again, write EVERY SINGLE PIECE OF LOGIC that we use as a publicly replaceable/accessible method. This gives people the ability to monkey patch and extend behavior.

I cannot agree. I believe it's designer responsibility to carefully decide what can be replaced. Otherwise a) no method could refer to outer scope and b) update that component on a project would not be an easy task.

@ProLoser
Copy link
Member

a) having a directive refer to outer scope is extremely, like ridiculously, bad design. You should use isolate scope or $parse instead to treat things internally.
b) I don't know why updating would be hard.

All I'm saying is instead of doing:

function x() {...}

you do

ctrl.x = function() {...}

That's it.

This means that all the logic that is intended to be used internally is externally exposed and properly documented regarding it's API. As long as the API is properly adhered to, then anyone who wishes to can extend, supplement or replace that logic with their own without us having to 'plan' for it. This is the basis of composibility, modularity and extendability. The biggest negative is probably around letting people directly access internal data/state such as the items or the visible state, however if we keep our methods small and concise and provide proper methods (setters) to handle even the mildest complexity (ctrl.choose = (item) => { ... }) then it shouldn't matter.

@awerlang
Copy link
Contributor

a) having a directive refer to outer scope is extremely, like ridiculously, bad design. You should use isolate scope or $parse instead to treat things internally.

Agreed. But I refer to lexical scopes, that is, closures.

Regarding point (b), it is that any bugs or features made into new releases that happen to alter public methods, each user is in charge of updating his/her code accordingly. If an user is encouraged to replace any method, changing any logic there, all updates are guarantees of breaking changes. It doesn't matter how much code coverage there's in component's or user code, it'll fail on integration.

Technical support would also be challeging. Versioning will be in a gray area, because a given version may function some way in one environment, yet not work in another. Instead of promote liberty to users, it could make the component less attractive. I believe instead a compromise should be taken, one that does not create breaking changes.

@meyerds
Copy link
Contributor

meyerds commented Sep 10, 2015

This appears to be a pretty old conversation, but I'd like to throw my hat in the ring here.

Another thing that would immensely improve the extendability of the design is to move all the DOM interaction out of uiSelectCtrl into link functions on the directives -- in addition to the improved API you have already been discussing.

Talk is cheap so here's my offer - I am willing to go and do that kind of refactor. But I don't want to step on any toes if there's already a big effort underway per the above conversation. @cdjackson & @ProLoser - I'd appreciate your take on this.

@ProLoser
Copy link
Member

@meyerds I haven't followed up on this recently (sadly) but I've been considering taking my own stab at rewriting this plugin because my company really needs it.

However, if I were to do it myself there's a good chance I'd be starting over from scratch because I'm fairly unhappy with the state of the code as it is, and it'd be extremely dumb. At most it'd have single select and multi-select and a powerful yet minimal API, but that's about it (similar to ui-mention which I recently created).

Poking around @cdjackson's fork it looks strong but there's just too much code for me to thorughly review myself and I'm not really seeing anyone else helping vet the code.

What exactly were you thinking of doing for your attempt at building this out?

If you guys are just cleaning this project up, I can try to review, but I can't contribute as the code base has gotten a bit larger than I have the time to dig through. If anyone else likes the idea of going back to square 1 and starting from scratch, hammering out some critical features, and enumerating all the secondary features we want to enable people to do themselves without explicitly doing in our code-base, then I can collaborate more heavily.

@meyerds
Copy link
Contributor

meyerds commented Sep 10, 2015

@ProLoser I'm in a similar situation with my company - I need to be able to extend the behavior. Specifically intercept keystrokes in the search input and conditionally let them propagate through. No way to do that currently because the keyup and keydown handlers are registered in the uiSelectCtrl instead of in a link function.

Anyway. The little bit that I've poked around the current codebase, I'd agree that starting over from scratch would be the ideal approach. Lots of issues there. Unfortunately, I don't think my client would pay for that scope of change. I could probably get them to buy off on fixing the current codebase just for my issue specifically.

The issue I've got isn't high priority to the client though, so if someone like you or @cdjackson are serious about starting over from scratch I'd be happy to contribute in whatever way makes sense. Even if on personal time. Seems like a worthwhile endeavor.

@ProLoser
Copy link
Member

@meyerds if I were to start over, you'd essentially have 2 elements, a dropdown and an input. And I would not make these part of the template. You'd have to insert the dropdown yourself, but it would provide you with a scope reference to the controller so you'd just do <a ng-click="$select.choose(item)"> etc. This would give you full control to do almost everything, except for multiple-select.

Then I'd want to implement multiple select which is trickier because the input widget is actually several DOM elements (it's a list of small anchors which can be highlighted, have cancel buttons, and the typeable area has to keep resizing to fill the remaining space). This would be isolated to a secondary directive that adds 'multiple' behavior to JUST the input part. I don't know if there are any other features I'd really care about, because if you have access to the native <input> you can toggle things like being enabled/disabled. Hook into click events, put your own conditions around the choose() function and can probably easily mess with things like the bindings if we do: $element.on('click', function(){ ctrl.click() }) which would allow you to override the ctrl.click property with your own variant.

@jnelson
Copy link

jnelson commented Sep 10, 2015

@ProLoser @meyerds I'd be happy to do fill-work bringing a new project following @ProLoser's simple OO approach up to parity via examples / extensions. If I had time I'd be in for the lib-side work, but I just can't commit to that in a reasonable timeframe right now. I'm following this issue; if you move the discussion, please ping me.

My project is stuck with my fork until something moves; not a situation I enjoy.

@ProLoser
Copy link
Member

I'm thinking perhaps we should start a new issue (or repurpose this one?) as a new breakdown of what the core features are, a loose api, and a 'nice-to-have' feature list and approximations of how these could be accomplished by mish-mashing our api together. I think the biggest flaw with my overly-simplistic approach is I tend to not opt for dealing with positioning or managing of the dropdown whatsoever.

@jnelson
Copy link

jnelson commented Sep 24, 2015

Seems to me that managing the dropdown (includes positioning) is a core responsibility in the sense that default behavior is necessary - can't demo without it - but superficially I think we could handle that in the same way (extend/override).

@ProLoser
Copy link
Member

@jnelson but think about bootstrap, which actually requires you to place the dropdown HTML onto the page and the relevant container. The only thing it really does is toggle visibility of the dropdown, but that's about it.

If you're looking for a do-everything-in-one-html-tag sort of directive we could wrap it but I'd rather just ask other people to wrap it in a proprietary directive so they can configure all the convoluted variations they wish into it.

@jnelson
Copy link

jnelson commented Sep 24, 2015

I happily lump "toggling visibility" in with "managing the dropdown" and concede that it could be nearly all of it. 👍

The way https://github.com/angular-ui/bootstrap/blob/master/src/dropdown/dropdown.js accomplishes its goal isn't crazy. It's not what we're discussing, but maybe it's an interesting reference point. This repo has all its select2 baggage, but my guess is that a large number of users aren't attached to that original intent.

I'm still a bit stuck on the wise idea to provide "functionality via documentation" of a simple, flexible API. That seems technically smart and might also be a nice contribution target. You could call it a state machine but I'd rather see a user-manipulable promise pipeline (or something better). Series of events, things can happen.

And then there's "build the core in pure JS, then wrap that project in an Angular adapter". (I haven't checked to see whether this exists yet.)

@wesleycho
Copy link
Contributor

Closing this issue - currently this project is being maintained by @user378230 and @aaronroberson , and I'm just going through and helping out with basic issue triaging/PR code reviews.

This library could use a lot of love though, so any high quality help is welcome, whether it is directly with helping create high quality features/bug fixes or creating up to date Plunkers and verifying or debunking issues.

@aaronroberson
Copy link
Contributor

Thank you so much for your help @wesleycho!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants