[CLOSED] Add lo-dash

[core-ai-bot]

Issue by gruehle
Monday Sep 16, 2013 at 20:24 GMT
Originally opened as adobe/brackets#5229

---

We should add Lo-dash to the Brackets core code. There are many places where we've re-implemented underscore-style functionality or could improve our code by using Lo-dash.

If you find a piece of code that could be better expressed using Lo-dash, please add it as a comment to this issue.

[core-ai-bot]

Comment by gruehle
Monday Sep 16, 2013 at 20:29 GMT

---

Dang, I wrote backbone in the original bug report, but really meant underscore 😊. Title and description updated.

[core-ai-bot]

Comment by gruehle
Monday Sep 16, 2013 at 20:30 GMT

---

Async.whenIdle() is the same as underscore debounce(), and could be eliminated.

[core-ai-bot]

Comment by iwehrman
Wednesday Sep 18, 2013 at 22:44 GMT

---

1. utils/StringUtils.htmlEscape could be replaced by _.escape.
2. utils/NumberUtils contains only getRandomNumber, which could be replaced by _.random.
3. Better yet, DocumentManager._untitledDocumentPath could use _.uniqueId instead of the aforementioned random number facility.
4. There are a number of instances of setTimeout(fn, 0) that could be replaced by _.defer(fn). (But asap would be even better.)
5. There are a few of instances of array.slice(0) that could be replaced by _.clone(array).
6. CollectionUtils.hasProperty could be replaced by _.has.
7. CollectionUtils.some could be replaced by _.some.
8. CollectionUtils.forEach could be replaced by _.forEach.

[core-ai-bot]

Comment by dangoor
Monday Sep 30, 2013 at 19:07 GMT

---

We spoke about this issue today and collectively decided that the extra features in Lo-Dash make it worthwhile to add to Brackets. Specifically, we would take the full "modern browser" version of Lo-Dash.

Also of note: we are going to separately consider the issue of using Lo-Dash for templating rather than Mustache. For now, we're just going to use the non-templating features of Lo-Dash.

[core-ai-bot]

Comment by njx
Monday Sep 30, 2013 at 19:15 GMT

---

Removing "needs review". Assigning to me/medium priority for now as I'm interested in doing this, but if someone else would like to grab it feel free.

[core-ai-bot]

Comment by iwehrman
Tuesday Oct 01, 2013 at 18:13 GMT

---

Do we want to load this as an AMD module or as a global? (My vote is for an AMD module, since we'll probably have to clean up our use of globals eventually anyway.)

[core-ai-bot]

Comment by njx
Tuesday Oct 01, 2013 at 18:19 GMT

---

That seems fine to me. I assume that means we have to do _ = require("thirdparty/lodash/lodash") or some such at the beginning of each file? A little bit of a pain, but I've never liked having globals in general anyway.

[core-ai-bot]

Comment by iwehrman
Tuesday Oct 01, 2013 at 18:34 GMT

---

If we want we can use the module name, or name it ourselves, so that we just have to do _ = require("lodash"), which might be slightly less annoying. Actually I think having "logical" module names for third-party libraries will be good for modularity too because it doesn't tie us to a particular library location and file name.

[core-ai-bot]

Comment by njx
Tuesday Oct 01, 2013 at 19:00 GMT

---

👍

[core-ai-bot]

Comment by iwehrman
Tuesday Oct 01, 2013 at 19:12 GMT

---

I don't know if I'll have time to finish this before I leave for PTO tomorrow, but here's a branch that adds Lo-Dash as a module named "lodash" and takes care of a single VanillaJS-to-Lo-Dash conversion. I generated the lodash file using lodash-cli as follows:

lodash modern --minify exports=amd

This generates a minified AMD module that assumes modern browsers.

[core-ai-bot]

Comment by njx
Wednesday Oct 02, 2013 at 23:22 GMT

---

One thought: we might want to deprecate the utility functions we no longer need rather than removing them right away, in case other folks are using them. (I would guess no one is using this NumberUtils one, but if we started using _.escape() instead of StringUtils.htmlEscape(), for example, that might break some stuff.) Or we could just make our utility functions call the lodash versions.

[core-ai-bot]

Comment by dangoor
Thursday Oct 03, 2013 at 01:34 GMT

---

I think we should deprecate our existing functions. Hopefully people will test with the essentially equivalent lodash function.

[core-ai-bot]

Comment by njx
Wednesday Oct 16, 2013 at 19:08 GMT

---

@iwehrman - any interest in updating your branch to deprecate the old function (rather than removing it for now) and making a pull request?

[core-ai-bot]

Comment by iwehrman
Wednesday Oct 16, 2013 at 19:24 GMT

---

Sure. By "deprecate" do you mean redefine, e.g., StringUtils.htmlEscape to call _.escape and then console.warn about its usage?

Also: do you have a problem with removing NumberUtils in particular instead of deprecating it? I can't find any uses of that module using@dangoor's BracketsExtensionGrabber utility.

Reassigning to@iwehrman.

[core-ai-bot]

Comment by njx
Wednesday Oct 16, 2013 at 19:39 GMT

---

If nobody is using NumberUtils, then yeah, we can probably just remove it (and release-note it). In general, though, I think it would be fine to make existing functions just call the lodash version (and warn) as long as the semantics are the same.

[core-ai-bot]

Comment by iwehrman
Tuesday Oct 22, 2013 at 18:27 GMT

---

Fixed by #5539.