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

The future of ng-annotate #245

Open
olov opened this issue Jun 8, 2016 · 39 comments
Open

The future of ng-annotate #245

olov opened this issue Jun 8, 2016 · 39 comments
Labels

Comments

@olov
Copy link
Owner

@olov olov commented Jun 8, 2016

I have maintained ng-annotate for almost three years now and I think it's now time for me to either pass the maintainer bit forward or to declare ng-annotate feature complete, disabling implicit matching.

I created ng-annotate because it seemed tedious and error-prone to keep two parameter-lists in sync. At the time most angular-applications had similar enough structure that detecting functions to annotate automagically seemed like a good idea. But as angular progressed and its community grew, the slope became more and more slippery. I eventually added support for explicitly marking up which functions to annotate (first via /*@ngInject*/, then via "ngInject") but regrettably to late. The automagic expectation still remains, as reflected in the issue tracker. Please note that "ngInject" works great in practice - it takes a couple of seconds to type it in, you don't need to keep two parameter lists in sync and it's crystal clear which parts of your code will be transformed and which won't.

Which brings us to TypeScript and ES2015+ support. With "ngInject" markup it works fine as long as ng-annotate is fed with the tsc/babel/.. processed output. With implicit matching it's a whole different beast on top of the already slippery automagic-slope. I've consistently said no to adding ES2015+ support for this reason, and there are lingering pull requests and issues (mostly feature requests).

I see two paths forward (are there other options?):

  1. I will ship a vastly simplified ng-annotate 2.0, supporting explicit "ngInject" matching only. It will be (mostly) feature frozen but bugs will be fixed. An unimportant but still nice side effect would be that ng-annotate becomes even faster.
  2. I pass it along. The new maintainer(s) can revisit my decision to not support unprocessed ES2015+ input and how far to further slip down the implicit slope.

Thoughts on this? I'm interested in hearing from users, current and future contributors and from anyone interested in taking on the maintainer role.

@olso
Copy link

@olso olso commented Jun 8, 2016

@olov First of all thanks for keeping us up to date

I like path 1

@mgol
Copy link

@mgol mgol commented Jun 8, 2016

I like the first option - very reliable, less surprises.

All that'd need to happen going forward is to support new ECMAScript features where needed - for now injecting arrow functions & ES6 classes.

@jeremypele
Copy link

@jeremypele jeremypele commented Jun 8, 2016

@olov Thanks for the feedback. I would go for the 1st option.

Besides, it doesn't prevent anyone to fork this repo and support ES6.

@rasenplanscher
Copy link

@rasenplanscher rasenplanscher commented Jun 8, 2016

@olov I agree with the others, option 1 is the way to go. as Jeremy says, there's nothing stopping anyone from going forward anyway.

On the point of the slippery slope: I've found myself in situations where I wasn't sure if I could leave out the explicit annotation, and as with other instances of erring on the sides of consistency and clarity, I was on the path to annotating everything anyway. After all, even if I knew exactly when an un-annotated function would be injected and when not, why keep those guessing who come after me? Just my two cents 👍

Oh, and thank you for ng-annotate 😄

@matthieusieben
Copy link

@matthieusieben matthieusieben commented Jun 9, 2016

Go for option 1! As long as #220 is included

@vperron
Copy link

@vperron vperron commented Jun 10, 2016

Option 1. Unprocessed ES6 shouldn't be supported in my opinion. Use toolchains, dudes.

@mgol
Copy link

@mgol mgol commented Jun 10, 2016

Unprocessed ES6 shouldn't be supported in my opinion. Use toolchains, dudes.

Why? There are lots of ES6-compatible environments now and not every project has to support IE. Also, it's perfectly fine to want to run your code in ES6 mode during development for easier debugging and having ng-annotate work there would help a lot.

Requiring transpiling ES6 to ES5 in all cases decreases usefulness of ES6 a lot.

@olov
Copy link
Owner Author

@olov olov commented Sep 15, 2016

Alright, I think the timing is right to get this done now. @schmod what's the current status and how would you prefer to proceed with the migration? I'm thinking a prominent message at the top of the ng-annotate README that briefly explains that ng-annotate is now deprecated in favor of babel-plugin-angularjs-annotate, preferably with a link to a .md file on your repo that explains the easiest way to migrate (for existing ng-annotate users). The babel-plugin-angularjs-annotate README should also be updated to make it clear that it's the ng-annotate successor. If it's possible to disallow new issues on the ng-annotate github page then that will happen too. Anything else we need to do?

@schmod
Copy link

@schmod schmod commented Sep 16, 2016

I think the steps forward are:

  1. Bump babel-plugin-angularjs-annotate to 1.0.0 and turn implicit matching off by default
  2. Write a migration guide, and update README.md to reflect that we're considering the new project to be "stable" -- I'd like to PR this and ask a few of you to look it over before I merge it.
  3. Update ng-annotate's README.md to point new users to the Babel plugin.

I might have some time for this in the coming week. Apologies that I've been busy lately.

@schmod
Copy link

@schmod schmod commented Sep 28, 2016

I've taken a stab at updating the documentation for babel-plugin-angularjs-annotate in preparation for the 1.0.0 release.

https://github.com/schmod/babel-plugin-angularjs-annotate/tree/next

Proofreading and improvements are welcome ;-)

@olov
Copy link
Owner Author

@olov olov commented Oct 1, 2016

@schmod great - let me know when you're ready and I'll update the ng-annotate README with a deprecation notice and a link to your plugin. I'll also push a (final) release of ng-annotate to npm so that the updated README is there as well.

@herrherrmann
Copy link

@herrherrmann herrherrmann commented Dec 9, 2016

Thanks to much for the great work, @olov and @schmod! This is a prime example of how open-source software should happen. I'll gladly migrate my Angular projects to use the new babel plugin now and will let you know (in the new repo) if I run into any problems.
(Sorry if this is considered spam for some people who get mail notifications for this thread but I just wanted to express my gratitude.)

@mgol
Copy link

@mgol mgol commented Dec 9, 2016

One thing to consider: in the typical Angular 2+ setup used via Angular CLI if you want to mix in Angular 1 components, you won't be able to use ng-annotate as the Babel plugin doesn't understand TypeScript. But the current ng-annotate doesn't understand TypeScript as well so this is not making matters worse.

@schmod
Copy link

@schmod schmod commented Dec 9, 2016

In that scenario, you'd probably want to opt for the TypeScript compiler to output ES6, and then run the output through babel to transpile to ES5. (Or, alternatively, there's nothing stopping you from running babel against ES5 sources)

bluetech added a commit to bluetech/ng-annotate-loader that referenced this issue Apr 28, 2017
ng-annotate is no longer maintained[1], and hence fails when applied to
source code containing modern JavaScript constructs, like import and
export. In particular, this prevents usage with Webpack's ES6 modules
support (issue andrey-skl#17).

[1] olov/ng-annotate#245

To work around it, allow the user to specify a fork of ng-annotate,
which adds support for new JS syntax.
@mgol
Copy link

@mgol mgol commented May 19, 2017

@olov @schmod Can we proceed with the ng-annotate deprecation? I think the Babel plugin works really well right now and most people don't know ng-annotate is effectively deprecated and that they should move to the Babel plugin.

@olov
Copy link
Owner Author

@olov olov commented Jun 1, 2017

It is done. Thanks everyone! I'm leaving this issue and #253 open for historical reasons.

@mgol
Copy link

@mgol mgol commented Jun 1, 2017

@olov Can you run:

npm deprecate ng-annotate "My deprecation message"

so that people installing the package see the warning?

@schmod
Copy link

@schmod schmod commented Jun 1, 2017

I'll try to find time this week to update my repo to reflect ng-annotate's deprecation (and finally get 1.0 out the door)

@mgol
Copy link

@mgol mgol commented Jul 23, 2017

@schmod Any updates? :)

@Dashon-Hawkins
Copy link

@Dashon-Hawkins Dashon-Hawkins commented Aug 24, 2017

I really like and respect this community of commenters and in particular @schmod. I'm a newbie developer and was taught ES5 and now trying to transition into more functional programming in Javascript using ES6. I'm still have some minor difficulties wrapping my head around it. What @schmod has created is really great and I have been highly anticipating v1.0. Any word yet on a timeline? I would also like to contribute to any open source project based upon my beginner skill set. Any recommendations? I don't care how mundane or minor my role is. I just want to be more active in the community and grow and learn.

@asednev
Copy link

@asednev asednev commented Mar 14, 2018

Here's a fork that has been patched to work with imports/exports in ES6: https://github.com/bluetech/ng-annotate-patched

@asednev
Copy link

@asednev asednev commented Mar 15, 2018

There is another fork from nevcos that supports ngInject for ES6 classes. It's little behind bluetech fork but useful in own way.

Also, if you use ng-annotate with gulp, take a look at gulp-ng-annotate-patched

@FRSgit
Copy link

@FRSgit FRSgit commented Feb 18, 2019

Actually, I've improved nevcos fork a little and everything got merged to ng-annotate-patched - feel free to use it with working support for ES6 classes.
BTW shouldn't ng-annotate-patched be mentioned in this library README file (and maybe npm deprecation message), so others can find it more easily?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.