Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace babel-plugin-angularjs-annotate with babel-plugin-inject-args #2700

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

robertknight
Copy link
Member

It is confusing to have a Babel plugin with an Angular-related name and
Angular-sounding annotations (@ngInject) in the code, even though
we're not using AngularJS. Although we're not using AngularJS any more,
we still have services that are instantiated by a dependency injection
container. The container reads dependency names from a $inject
property on service functions/classes. This $inject property is added
by babel-plugin-angularjs-annotate.

This commit replaces the angularjs-annotate Babel plugin with one
maintained by us [1]. The new plugin provides only the functionality that we
need (eg. only processes explicitly annotated functions) and uses a more generic
@inject annotation.

[1] https://github.com/hypothesis/babel-plugin-inject-args

It is confusing to have a Babel plugin with an Angular-related name and
Angular-sounding annotations (`@ngInject`) in the code, even though
we're not using AngularJS. Although we're not using AngularJS any more,
we still have services that are instantiated by a dependency injection
container.  The container reads dependency names from a `$inject`
property on service functions/classes. This `$inject` property is added
by `babel-plugin-angularjs-annotate`.

This commit replaces the `angularjs-annotate` Babel plugin with one
maintained by us [1]. The new plugin provides only the functionality that we
need (eg. only processes explicitly annotated functions) and uses a more generic
`@inject` annotation.

[1] https://github.com/hypothesis/babel-plugin-inject-args
@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #2700 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2700   +/-   ##
=======================================
  Coverage   97.73%   97.73%           
=======================================
  Files         200      200           
  Lines        7497     7497           
  Branches     1647     1647           
=======================================
  Hits         7327     7327           
  Misses        170      170           
Impacted Files Coverage Δ
src/sidebar/cross-origin-rpc.js 100.00% <ø> (ø)
src/sidebar/services/analytics.js 100.00% <ø> (ø)
src/sidebar/services/annotations.js 98.38% <ø> (ø)
src/sidebar/services/api-routes.js 94.11% <ø> (ø)
src/sidebar/services/api.js 92.42% <ø> (ø)
src/sidebar/services/autosave.js 100.00% <ø> (ø)
src/sidebar/services/features.js 100.00% <ø> (ø)
src/sidebar/services/frame-sync.js 96.87% <ø> (ø)
src/sidebar/services/groups.js 100.00% <ø> (ø)
src/sidebar/services/load-annotations.js 100.00% <ø> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df52a1f...a9f027d. Read the comment docs.

Copy link
Contributor

@lyzadanger lyzadanger left a comment

Choose a reason for hiding this comment

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

Cool. I can build this and it works.

My only feedback is that@inject, while totally accurate and appropriate, is not particularly unique. For example, as a dev, I could see myself being like "what magic does this perform" and trying to google for inject in the code/dependencies. If it had a more specific, albeit cumbersome, name, like @babel-inject, it might be easier to track back the magic to its source. I know you've already had a naming runaround, though...

@robertknight
Copy link
Member Author

My only feedback is that@inject, while totally accurate and appropriate, is not particularly unique.

That's a fair point. @babel-inject suggests to me that it is a built-in feature of Babel rather than a custom thing. I'd like to get this merged and then if someone can suggest an alternative which we collectively like it will be easy to update the plugin to support it.

@robertknight robertknight merged commit cacdd63 into master Nov 4, 2020
@robertknight robertknight deleted the replace-angularjs-annotate-plugin branch November 4, 2020 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants