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

fix(loader): support v1.6.3+ (and other fixes) #308

Closed
wants to merge 4 commits into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Mar 23, 2017

  • Updated dependencies and configs.
  • Switched to yarn.
  • Added support for the angular.Module#info() method, necessary in v1.6.3+.

Fixes #307.

* @license AngularJS v1.3.0-build.3042+sha.76e57a7
* (c) 2010-2014 Google, Inc. http://angularjs.org
* @license AngularJS v1.6.4-build.5322+sha.d96e58f
* (c) 2010-2017 Google, Inc. http://angularjs.org

Choose a reason for hiding this comment

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

Is this stuff just taken straight from the 1.6.4 loader or is it modified?

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 is modified in a couple of places (e.g. minErr doesn't have access to toDebugString() so it uses a simpler implementation and there are a couple of other changes (marked with ANGULAR HINT ALTERATION comments) to support 1.2.x as well.

Maybe it is better to remove the header and leave a comment that it is based on v1.6.4 loader.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

(BTW, reviewing with ?w=1 reduces the noise significantly 😃)

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 just realized the original comment was about dist/hint.js. My responses were about loader.js.
dist/hint.js includes loader.js (among othr things).

I had accidentally commited dist/hint.js and the loader.js changes on different commits (although the dist/hinst.js changes are directly affected by the loader.js changes). I have now amended squashed the two commits into one. Sorry for the confusion 😁

Copy link

@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.

I have no idea where the changes come from but I guess they are generated in some way.
Does Batarang have any tests?
So in my ignorance LGTM.

@Narretz
Copy link
Contributor

Narretz commented Mar 24, 2017

The hint.js is copied from angular-hint. :)

@gkalpak
Copy link
Member Author

gkalpak commented Mar 24, 2017

Yes, hint.js is generated based on angular-hint's hint.js (as published on npm). I copied the latest loader.js from angular/agular.js and tweaked as necessary.

Batarang has some tests, but afaict they are about the DevTools panel, rather than how the extension interacts with AngularJS apps. So, stuff like that would not be caught.

@gkalpak gkalpak force-pushed the fix-loader-support-info branch from c369f62 to e9efcce Compare March 24, 2017 12:46
@gkalpak gkalpak closed this in 9684d8d Mar 24, 2017
@gkalpak gkalpak deleted the fix-loader-support-info branch March 24, 2017 12:55
@gkalpak
Copy link
Member Author

gkalpak commented Mar 24, 2017

Merged 😁 (Happy to address additional feedback/comments in follow-up PRs.)

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.

3 participants