Skip to content
This repository has been archived by the owner on Feb 9, 2020. It is now read-only.

Fix crash introduced in v0.10.8 and slightly improve loader #311

Closed
wants to merge 3 commits into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Apr 2, 2017

This PR includes:

Fixes #88, #310.

@@ -101,8 +109,7 @@ function bufferData(tabId, message) {
// context script –> background
chrome.runtime.onMessage.addListener(brokerMessage);

chrome.runtime.onConnect.addListener(function(devToolsPort) {

chrome.runtime.onConnect.addListener(function (devToolsPort) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is it with adding spaces after function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It falls under the random changes category 😃
Both variations are used throughout the project - it drives me crazy. I was trying to blend in with the rest of the file 😁

@@ -71,13 +72,20 @@ function bufferData(tabId, message) {
}

if (message.event === 'scope:new') {
tabData.scopes[message.data.child] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note that this change no longer modifies the tabData.scope object. Is that on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does. I just stored some properties as local var (childScopeData = tabData.scopes[childId]).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At no point do you write to tabData.scopes[message.data.child] any more

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the childScopeData never gets assigned onto the tabData.scopes object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, you are right 😱

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this was used to "hydrate" the Scopes view when the DevTools would open, at which point the DevTools app would take over maintaining the scopes info. With my incorrect change, the scopes would not be hydrated when you opened DevTools, so you needed to refresh the app with the DevTools open.

Thx for watching out for my blunders 😃
It really sucks that there are no substancial tests. Hoping to fix it some time.

hint.js Outdated
severity = data;
data = event;
event = this.event;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wot no tests? :-P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, right? :P

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming that the scopes modification thing is not a thing - i.e. I am wrong; or I am right and it doesn't matter.

@gkalpak
Copy link
Member Author

gkalpak commented Apr 4, 2017

Fixed the scopes thing in 68f8830. Thx!
Merging...

Somewhere between v0.14.4 and v4.0.0, `EventEmitter2` started being inconsistent
wrt to the arguments passed to the `onAny()` listener; sometimes the `eventType`
is passed as the first argument, sometimes it isn't. This caused batarang to
crash.

This commit fixes it, by accounting for the fact that `eventType` might or might
not be passed as a first argument.

Fixes angular#310
…d `minErr`

This commit updates `loader.js` based on `angular-loader.js` from
angular/angular.js#15881, which is essentially the latest loader plus some
`minErr` fixes (see angular/angular.js#15881 for more info).

Additionally, the loader does a better job staying closer to the original
`angular-loader` behavior for versions 1.3+ and it only modifies its behavior to
support 1.2 apps. This change also fixes previously broken usecases that rely on
private `angular.Module` APIs (see angular#88 for more info).

Fixes angular#88
@gkalpak gkalpak force-pushed the fix-loader-small-improvements branch from 68f8830 to 1e5bd10 Compare April 4, 2017 07:09
@gkalpak gkalpak closed this in f04d250 Apr 4, 2017
@gkalpak gkalpak deleted the fix-loader-small-improvements branch April 4, 2017 07:11
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.

None yet

2 participants