Live Development -- Provide API for enabling/disabling Agents #1303
Conversation
…ns can use the more experimental agents
ghost
assigned 
| @@ -221,25 +230,48 @@ define(function LiveDevelopment(require, exports, module) { | |||
|
|
|||
| /** Unload the agents */ | |||
| function unloadAgents() { | |||
| var i; | |||
| for (i in agents) { | |||
| _loadedAgents.forEach(function (i) { | |||
There was a problem hiding this comment.
The first arg here is the value of _loadedAgents[i], not i itself. As-is I think this code will never try to unload anything.
There was a problem hiding this comment.
Oops, never mind... now that I've read below I see _loadedAgents is actually an array of agent names (the keys of 'agents'), not the agents themselves. Could we clarify that either by renaming this array or at least by adding docs where it's declared? Also, similar to below let's not use 'i' here for something that's not actually an index.
(Alternatively, you could change _loadedAgents to be an array of the actual agent objects... that seems fine to me too).
Also, the hasOwnProperty() check can go away now that you're using forEach().
There was a problem hiding this comment.
Changed name of _loadedAgents to _loadedAgentNames to make this more clear. Also added a comment.
And, I removed the hasOwnProperty check.
|
@joelrbrandt: Ok, done reviewing |
|
@peterflynn Thanks for the review! Back at you. |
| agents[i].unload(); | ||
| } | ||
| } | ||
| _loadedAgentNames.forEach(function (name) { |
There was a problem hiding this comment.
Do you still need the check to see if unload() exists before calling it? I notice the check for load() existing is still present below...
There was a problem hiding this comment.
In the loadAgents() function, we're reading from '_enabledAgentNames'. Extension developers can put any string they want into this map (through enableAgent). That means that an agent might not actually exist with some name in _enabledAgentNames.
That's why we need the check in the loadAgents() function.
However, we only add a name to the _loadedAgentNames array if an agent with that name a.) exists and b.) has a 'load' function. All agents that have a load function should have an unload function.
So, when we're in unloadAgents and we do agents[name].unload(), an exception is appropriate if unload is undefined (or if agents[name] is undefined)
There was a problem hiding this comment.
After writing that, it seems like I should probably be checking in enableAgent to make sure that the agent they specify actually exists.
That'll remove the need to check agents.hasOwnProperty(name) in loadAgents. But, we still need to check for load() existing in the agent. It's possible that some agents might not have load/unload.
However, if an agent has 'load', it needs to have 'unload'. So, the unload test still isn't needed.
There was a problem hiding this comment.
change made in 1bce0f3
|
@peterflynn back at you again. Thanks! |
|
Sounds reasonable to me -- merging now. |
Live Development -- Provide API for enabling/disabling additional Agents
Provides an enableAgent/disableAgent API in LiveDevelopment so that extensions can enable more 'experimental' agents.
Satisfies goals of #1301, but doesn't enable experimental agents by default (i.e. their 'load' functions won't get called).