Creating Menu Item with an existing ID breaks brackets (in general: extensions that crash on init block Brackets startup)

[mikechambers]

If you try to have an extension that tries to create a menu item with a menu id that has already been used, you get the following error in Brackets

Uncaught Error: MenuItem added with same id of existing MenuItem: convert.uppercase Menus.js 243

and Bracket will not complete loading.

We should check for id collision and error gracefully so Brackets still loads and is useable.

[peterflynn]

It seems like a problem if any kind of exception thrown during extension init (not just calls into menu APIs) brings down the whole Brackets startup. Looks like this is a regression -- a plugin containing a simple throw new Error() breaks Brackets startup today, but in the sprint-8 build it works fine. It seems like plugins are being loaded earlier than they used to.

Regardless of how soon they load though, we should be able to wrap a try/finally around that bit or something.

[peterflynn]

I looked into catching extension-loading errors in general, and it turns out to be pretty hard. First, we have a few bugs with missing fail()/always() handlers on various Promises in the extension loading chain. But second, it's very challenging in RequireJS to get notified when an error occurs trying to load/init a module.

RequireJS seems to be philosophically opposed to per-module (in our case per-extension) load/init error handling (1) (2). Nonetheless, the current RequireJS docs make it seem like you should be able to do both per-module and global error handling. But I tried various permutations of those two mechanisms (along with the seemingly undocumented catchError flag) and have had no success getting any kind of notification when the module's define() function throws an error. It's as if the module just takes infinite time to load. We could work around this with callbacks, but if there's a way to use the real RequireJS error handling it'd sure be nicer.

We should definitely fix that general issue at some point, but it seems potentially time consuming. If this menu issue feels more urgent, one way we could fix it is by making it a non-fatal error -- addMenuItem() could log an error to the console and then no-op, much the same way KeyBindingManager.addBinding() does for shortcut collisions. The downside is that if the dev doesn't keep an eye on the console open, the API call would appear to just silently fail to create a menu item with no explanation. Personally I'd actually prefer it to fail hard with an exception... but maybe only if Brackets is more robust to exceptions during extension loading.

[jasonsanjose]

Related to #961.

I'll file a separate bug about changing extension loading so that it doesn't crash Brackets at startup.

[tvoliter]

Marking sprint 10 since this is related to #961 which is also marked sprint 10.

[pthiess]

Reviewed

[redmunds]

Fixed in tvoliter/context-menus branch

[redmunds]

FBNC to @mikechambers This should now add a message to console log, but not throw an exception.

[mikechambers]

confirmed fixed.

[peterflynn]

Followup to my long comment above on Require's error handling... We've now migrated to RequireJS 2, which provides per-require() "errbacks" for much better error handling capabilities. Pull #1968 makes us much more robust to errors that occur during extension loading.