Code Audit

[ProLoser]

Hey guys, this repo pops up in discussion, and I realize with 400+ issues you are barely able to keep up with the onslaught of issues. I did some poking around and found the distributed un-minified file size is 74kb. That's nearly half of ui-bootstrap's filesize.

I poked around the src files and I have to say I'm completely lost most of the time.

I think (if it's at all possible) it's time to streamline this project and get rid of and merge as many bells and whistles as possible. I realize this is one of the most complex widgets that tend to pop up (aside from a wysiwyg editor or a grid) but if ever there was an opportunity for simplification, this project could merit it.

Not that I have been keeping up-to-date as to the latest trends of the project, but here are some examples:

1. Use flexbox instead of JS sizing the input box.

2. Choice locking? Disabled choices? Choice-related garbage? Nah, lets git rid of all that from the core

3. Implement lifecycle callback hooks: beforeSelect, afterSelect, beforeRemove,afterRemove. If we use these, it's ridiculously easy for developers to implement 2 however the hell they want.

4. Remove repeat attribute (+ the parser)? This may be more difficult, but what if we stop making <select-choices> represent 1 row and instead put the onus on the developer to handle repeating? What if you had to specify the <ul> yourself too? No more passing of classes or HTML, no more positioning or styling garbage. No more grouping or sorting or filtering garbage. The user entirely decides what he wants to show on the list, and what he wants to perform when the user clicks on a row. We simply relocate this logic to the templates.

5. Sortable should be distributed in a separate file. It should not come with the core. The whole point of repacking sub-features into sub-directives is so that it wouldn't bloat the core, it would reduce filesize, and it would teach / encourage other developers to create their own plugins instead of adding to the 400+ bugs on this repo.

6. Grouping should be a separate file / plugin. If we can't implement it as a plugin, our API needs to be improved.

7. What is this refresh stuff? Can't we remove it?

8. Get rid of angular.closests(). It's not even used in the repo, just in the tests!

9. Get rid of querySelectorAll() polyfill. It's in all the browsers that count and I don't even think the version of angular we're using supports prior browsers.

10. Take highlight out of the core. Make it part of the themes(?). It's a nice feature, but not really super critical and people can implement this however they want to with ease.

11. Add dependency to ui-position so we can get rid of uisOffset perhaps? I'm on the fence about this one. Technically, you don't even HAVE to position stuff via javascript:

    <div style="position: relative">
    <input />
    <ul style="position: absolute; top: 100%; left: 0;">...</ul>
    </div>

    I realize this isn't super amazing calculate side flipping and all that garbage, but make that a separate behavior if at all possible mayhaps?

12. Make closeOnSelect get called explicitly in afterSelect and just let people override it or something? Seriously, lifecycle callbacks ftw.

13. No idea what searchEnabled is for or why we're watching it. I feel a lot of stuff doesn't need to be watched, we could just evaluate these things upon interaction. Are you really going to change the searching ability while the user is in the middle of searching? If some weirdo REALLY wants to, lets expose the API so that it's super easy for them to override this behavior.

14. Why are we watching sortable too? This doesn't need to be kept up to date constantly, only when the user opens the input I would think.

15. tagging is really just a behavior that happens when the user types in an input. If we have some sort of lifecycle callback whenever the user hits enter that simply allows him to push his choice into the list of choices, like on-select then we're done. There is no more tagging flags. Same goes for choices, if we let the user render the choices how they see fit, they can specify in the template what 'new' choices look like (or don't).

16. taggingToken could quite literally be an ng-keypress event where the dev is able to explicitly call $select.items.push() or $select.select('new-choice') or something. Again, this should be offloaded into a third-party directive as a plugin.

These are just thoughts, and I'm sure I'm a big PITA and half of these aren't actionable, but I'd really like to see this repo get new life, by simplifying it and allowing devs to take their bugs into their own hands.

If anyone would be interested in stepping up and making these changes, feel free to reply!

[bunsenmcdubbs]

+1

[icfantv]

Addressing number 9 above, common.js is already using the native browser Element.querySelectorAll() in its prototype definition of querySelectorAll() on an angular.element. the call to this[0] returns an Element object.

So unless we want to get rid of the prototype, I don't really see anything to do here. That said, while there is a check to see if another framework has already added a prototype with the name querySelectorAll, there is no fallback, meaning, we implicitly trust another library's implementation if it already exists.

If we wanted to go this route (removing the prototype), we'd have to do something a little ugly. e.g.,:

angular.element($element[0].querySelectorAll('input.ui-select-search')[0])

Which, quite frankly, doesn't feel right. In this case, however, it is necessary to rewrap the result as an angular element because of how the ctrl.searchInput is being used in this example - see

ui-select/src/uiSelectController.js

Line 42 in 1e1d908

ctrl.searchInput = $element.querySelectorAll('input.ui-select-search');

for context. But, it may not be necessary for all uses - I haven't checked the entire code base.

[amcdnl]

Very much agree. Its almost a re-write at this point.

With Angular 2.0 on the verge of coming out, I'd like to hear what peoples feedback on using TypeScript or ES6 would be. I recently rewrote one of my own projects and ES6 just gives you sooo many new things you can use to write amazing JS code now.

Also, it should take into account many of the best practices that are published for Angular2 so the upgrade would be easy.

[ProLoser]

@amcdnl if you're referring to johnpapa's guide, a lot of the AngularUI team and others disagree with a lot of those practices.

Anyway, I personally have been quite pleased with leveraging ES6. I do not however agree with using TypeScript, but again whoever's putting in the most effort on these projects is free to make those decisions.

[amcdnl]

@ProLoser Not nesc that guide, there are a bunch of items floating around out there on how to easily convert up to Angular 2 given current apis, would just be nice to account for those in the design so we can upgrade easily when the time comes. I'm not in love with TS either, its annoying A2 is almost forcing it vs going w/ the standard but thats a different convo.

What do you think of a re-write vs just a re-org/refactor?

[icfantv]

I'm ok w/ a rewrite for ng2 fork (this is what Kent is doing w/ formly) - I've been looking for a reason to learn ES6. Would we still feel compelled to support the existing version? (I mean, it's not like we were looking for more work, right?)

BTW, @amcdnl, please let me introduce myself. I'm Adam and @ProLoser invited me to join the project as we use it at work (single & multiple selects and tag fields).

[ProLoser]

I would rather see ng1 cleaned up first and foremost. I believe the
optimizations I am prescribing will not impede the transition in any way
because they are about general choose organization vs angular practices.

I do not expect demand to do for this version anytime soon.

On Thu, Jul 23, 2015, 4:12 PM Adam Gordon [email protected] wrote:

I'm ok w/ a rewrite for ng2 fork (this is what Kent is doing w/ formly) -
I've been looking for a reason to learn ES6. Would we still feel compelled
to support the existing version? (I mean, it's not like we were looking for
more work, right?)

BTW, @amcdnl https://github.com/amcdnl, please let me introduce myself.
I'm Adam and @ProLoser https://github.com/ProLoser invited me to join
the project as we use it at work (single & multiple selects and tag fields).

—
Reply to this email directly or view it on GitHub
#1096 (comment)
.

[amcdnl]

@ProLoser Honestly though, I was trying to fix a few bugs I found the other day and there was so much code that was brittle I ended up not fixing them and just working around. This directive is a prime example, its nuts w/ all the IF branching - https://github.com/angular-ui/ui-select/blob/master/src/uiSelectMultipleDirective.js

@icfantv nice to meet you! I'm responsible for most of the bad grammar in the docs ;). I use it at work too in about 100+ places ;).

[ProLoser]

If you want to attempt a rewrite go ahead.

On Fri, Jul 24, 2015, 4:52 AM Austin [email protected] wrote:

@ProLoser https://github.com/ProLoser Honestly though, I was trying to
fix a few bugs I found the other day and there was so much code that was
brittle I ended up not fixing them and just working around. This directive
is nuts all the different IF branching -
https://github.com/angular-ui/ui-select/blob/master/src/uiSelectMultipleDirective.js

@icfantv https://github.com/icfantv nice to meet you! I'm responsible
for most of the bad grammar in the docs ;). I use it at work too in about
100+ places ;).

—
Reply to this email directly or view it on GitHub
#1096 (comment)
.

[mfeingold]

At some point I got a request to provide a way to use ui-ui-scroll to implement the dropdown part of the ui-select. The idea seems to be very reasonable, but after a brief look at the ui-select code I realized that it would be prohibitively difficult to do with the existing structure of the ui-select. As you guys are considering re-write, would you like to think about how can we marry the two? On my end I would.

[ProLoser]

@mfeingold that would be awesome. I have actually been building a @mention plugin for AngularJS that mirrors Facebook's implementation and will probably be releasing it to AngularUI as soon as it's done. I have been trying to focus on Composability in the code I write recently and feel that what I've built is a good starting point for something that is highly modular and extendable. All the behaviors are broken down into small, publicly exposed functions and I want to come up with a streamlined way to allow people to override them.

Here's a link to my WIP: http://jsbin.com/fagonu/edit?js,console,output

Note that I even forego messing with the DOM too much and even decline to actually generate the dropdown myself. I realize it's not feasible to expect the dev to create a dropdown list for every single textbox on the page but I am trying to make my directives have as little direct interaction with the DOM as possible. It simply updates values on itself and the DOM reacts via angular templates.

For that directive ^, it would be possible to customize or add a plugin by doing this:

directive('myMention', ($http) => {
  return {
    require: ['mention', 'myMention'],
    link($scope, $element, $attrs, [mention, myMention]) {
      angular.extend(mention, myMention);
    },
    controller() {
      this.pattern = /(?:\s+|^)!(\w+)$/; // !bob
      this.label = function(choice) { return `${choice.first_name} ${choice.last_name}`; };
      this.findChoices = function(match) { $http.get(...).then( response => this.choices = response.data ) };
    }
  };
})

Although this isn't 100% perfect, it's another pattern I want to start pushing into the community as an alternative way to 'enhance' plugins with their own variations. This is how we will introduce modularity and composibility into the community.

[amcdnl]

@mfeingold Have you seen angular-material has implemented a virtual repeat that is similar to ui-scroll - https://material.angularjs.org/HEAD/#/demo/material.components.virtualRepeat

[mfeingold]

@ProLoser I am of the same opinion. I think building huge (complex, heavy - pick one) monolithic components with gazillion options, bells and whistles can only take you so far. To build something truly versatile and expandable it needs to be composable. In case of the select component, wouldn't it be great if it were built so that the actual dropdown part would be defined by a plugin with a well defined contract, preferably on the level of the template. If this were the case, I would have a much better chance to wrap scroller as an alternative plugin to be used with the select component.

[mfeingold]

@amcdnl no I have not. It is cool that they support horizontal scrolling. The requirement that all elements should have the same size defined upfront reduces the usability though. Also it is not dynamic in the sense that the entire dataset has to be pre-loaded

[ProLoser]

@mfeingold In ui-mention the dropdown isn't actually the responsibility of the plugin to build, it's up to you as the developer to build yourself. It simply provides you with the data to iterate over and some helpful methods. I suppose if you have 200 rows and you want to do focus on scroll and other crap like that it's a little more complex but at least you have the ability to implement it as you see fit. I personally find all those bells and whistles superfluous when you can just do things like show the first 5 results, etc.

[jrbotros]

@ProLoser I'd love to set up Webpack/Babel/ESLint so we can write things in ES6 and maintain a more consistent style—should I just go for it and submit a PR, or do you think it warrants more discussion first?

[ProLoser]

Go for it.

[ProLoser]

My feelings on typescript have changed as of late to keep in step with the angular core. Whomever is working on this project should feel free to push the project in the direction they see fit since it's not like I'm actually doing it lol