adobe / brackets

An open source code editor for the web, written in JavaScript, HTML and CSS.
http://brackets.io
MIT License
33.25k stars 7.63k forks source link

Clean up require() dependencies in brackets.js #1783

Open njx opened 12 years ago

njx commented 12 years ago

The dependencies in brackets.js need some cleanup.

  1. There are a number of modules that are assigned in the requires up top, but aren't used anywhere in the file. It's hard to tell whether these are actually necessary (because they need to boot a module but aren't actually referenced from brackets.js) or whether they could be removed.
  2. Some modules are required at the top, but only referenced from the brackets.test declaration. These requires should be moved into the brackets.test declaration (as long as they aren't required by case (1)).
  3. There are some hidden dependencies. I tried to move the require of "QuickOpen" to the list of requires() that doesn't actually assign the modules to variables. But moving it after the require of Menus breaks, because Menus adds menu items that seem to depend on the QuickOpen command being defined. We should either remove this dependency or make it explicit.
peterflynn commented 12 years ago

Hmm, (3) is a good point. Menu items can't be added until their command ID exists (i.e. is registered with CommandManager), so we almost need to ensure that Menus.js is loaded after everything else. It's probably working partially through luck right now...

One fix would be to make Menus.js depend on every module whose command is being referenced (which sort of makes sense but seems a pain to maintain). Or we could add menu items where the command is registered (like an extension does), but then our menu structure is much less declarative.

njx commented 12 years ago

We should at least move the actual instantiations of the built-in menus out of Menus.js (which should just be the menu manager code) and into an app-specific module. That module can require whatever it references.

pthiess commented 12 years ago

Reviewed assigned tio NJ