Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Add Lo-Dash #5539

Merged
merged 10 commits into from
Oct 17, 2013
Merged

Add Lo-Dash #5539

merged 10 commits into from
Oct 17, 2013

Conversation

iwehrman
Copy link
Contributor

This pull request addresses #5229 by adding Lo-Dash as a third-party dependency and deprecating a number of internal functions in favor of their corresponding Lo-Dash functions. In particular, this pull performs the following conversions and deprecations:

  1. Convert instances of Array.slice(0) to _.clone()
  2. Convert the lone instance of NumberUtils.getRandomNumber() to _.random and removes NumberUtils.js
  3. Convert StringUtils.htmlEscape to ._escape and deprecates the former
  4. Convert CollectionUtils.indexOf to _.findIndex and deprecates the former
  5. Convert CollectionUtils.forEach to _.forEach and deprecates the former
  6. Convert CollectionUtils.some to _.some and deprecates the former
  7. Convert CollectionUtils.hasProperty to _.has and deprecates the former

Functions are deprecated by adding a @deprecated annotation, and by replacing their implementation with the corresponding Lo-Dash function and a console.warn message. All the unit tests look good.

I'll let @gruehle decide if he wants to replace Async.whenIdle with _.debounce. I also didn't replace instances of setTimeout(..., 0) with _.defer because the Lo-Dash implementation isn't worth the trouble. (asap does it better.)

CC @njx

@gruehle
Copy link
Member

gruehle commented Oct 17, 2013

I'm in favor of replacing Async.withTimeout() with _.debounce().

@peterflynn - thoughts?

@iwehrman
Copy link
Contributor Author

I've updated the pull request to also completely remove Async.whenIdle in favor of _.debounce. I removed it instead of deprecating it because it's new and there are no extensions in the registry that use it. If this pull is merged, we can also close out #5528, despite the fact that that will break @gruehle's heart.

@ghost ghost assigned ingorichter Oct 17, 2013
Conflicts:
	src/editor/InlineTextEditor.js
@iwehrman
Copy link
Contributor Author

Fixed merge conflicts with master.

Conflicts:
	src/editor/MultiRangeInlineEditor.js
ingorichter added a commit that referenced this pull request Oct 17, 2013
@ingorichter ingorichter merged commit 86c4cc8 into master Oct 17, 2013
@ingorichter
Copy link
Contributor

@iwehrman Looked good! Merged.

@njx
Copy link
Contributor

njx commented Oct 17, 2013

💵 💵 💵

@lkcampbell
Copy link
Contributor

https://github.com/adobe/brackets/wiki/Brackets-Coding-Conventions#apis-to-use-or-avoid

probably needs an update in light of this change. For example, the part about using CollectionUtils.forEach() or CollectionUtils.some().

@iwehrman
Copy link
Contributor Author

I made the CollectionUtils -> _ update.

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.

5 participants