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

feat(plugin): support for ng-annotate, process .ng.js files #244

Merged
merged 1 commit into from
Mar 20, 2015

Conversation

nickjanssen
Copy link
Contributor

I know it's being discussed at #109 and meteor/meteor#3508 but it looks like we have to wait until they find a way to process the original .js files.

So hence my PR as a suggestion: why for now not register ng.js to be processable by ng-annotate? It would make my project a lot easier to work with.

In addition, we could do more processing on this extension such as https://github.com/werk85/grunt-ng-constant.

@Urigo
Copy link
Owner

Urigo commented Mar 19, 2015

@netanelgilad :) I think that's great and it is also optional so why not?
@nickjanssen thank you so much.
Question - do you think about a way to give people the option to use gulp or grunt to run any tasks they want on .ng.js ?
an option to make it more general?

@nickjanssen
Copy link
Contributor Author

@Urigo I'm not sure I understand - I don't think people should be using gulp/grunt at all themselves. Meteor/angular-meteor should care of this in its plugin system, just like it's currently already watching/compiling/minifying files.

As for an option to make it general...perhaps using a config json file? Like angular-meteor.json?
Personally I don't think it's necessary, I think the package should be smart enough to figure out on its own what needs to happen. And ng-annotate doesn't do any harm even if you don't need it, so why not let it run always?

@Urigo
Copy link
Owner

Urigo commented Mar 20, 2015

@nickjanssen yes that's true, so what if there is a default (with ng-annotate) and then only the programmers that will want to change that (for any reason and I think there might be a few as we can see the great usage and popularity of gulp and grunt) would be able to change that behaviour and use gulp?

@nickjanssen
Copy link
Contributor Author

Again, I don't think people should be using gulp/grunt here. That's Meteor's job. Gulp/Grunt are tools that compile an entire app by concatenation, minification, copying files to a dist folder, watching and serving them, which is exactly what Meteor already does for us. So, we should help Meteor so it is smart enough to handle those .ng.js files for us.

As for letting people choose their annotate compiler, that's too hardcore (and frankly, absurd. It's not like there are 100 ng-annotate alternatives). Better to choose convention over configuration in this case. ng-annotate is the norm and came after ng-min which perfectly replicates and improves the behaviour of its predecessor.

@Urigo
Copy link
Owner

Urigo commented Mar 20, 2015

@nickjanssen ok, great job!
merging

Urigo added a commit that referenced this pull request Mar 20, 2015
feat(plugin): support for ng-annotate, process .ng.js files
@Urigo Urigo merged commit a4a3455 into Urigo:master Mar 20, 2015
@Urigo
Copy link
Owner

Urigo commented Mar 23, 2015

@nickjanssen what's your Twitter handle (for the new version announcement)?

@nickjanssen
Copy link
Contributor Author

@pbastowski
Copy link
Contributor

Guys, I'm trying to integrate Babel into my workflow, but running into some trouble. So, what I'm after is: file.es6 --> babel --> file.js --> ng-annotate --> file.js

The annotate portion is why I'm asking in this thread. Maybe I should open a new thread?

So, what I did is:

  • I added the grigio:babel package to my project - all good so far.
  • Next, I created an arrow function and saw it transpiled into a normal ES5 function in the browser. Again, all good.
  • Then, I created an angular filter and injected $q into it - this is where things go a bit wrong - ng-annotate has not been ran on the transpiled code.

I have temporarily fixed this by copying grigio:babel and adding an ng-annotate step to it, immediatelu after the babel transpilation, which works very nicely indeed, for the moment. But, I would like to come up with a better solution, if at all possible.

Any suggestions?

@netanelgilad
Copy link
Contributor

The problem is with the current implementation of the plugin system that meteor uses. Every file extension can only have one plugin handling it.
That is why the babel package needs your files to end with es6 and not js (cause meteor handles js files) and that is why angular-meteor ng-annotate needs your files to end with .ng.js.
Hopefully this is an issue MDG is going to tackle soon, and we might be able to integrate more than one build plugin per extension.

@pbastowski
Copy link
Contributor

@netanelgilad Yes, that's also what I read about the current Meteor plugin design. Do you know if it is possible to define the order in which the plugins execute? I need Babel to run before ng-annotate, because ng-annotate does not understand ES6 as yet.

For the moment, we can work around the one file extension per plugin issue by simply calling several convertors (transpiler, annotation, etc) in sequence within the one Meteor plugin. I was also thinking about creating a generic plugin that would allow me to add several steps to it. Perhaps the plugin itself could have a "plug-in" design, that allows me to hook steps into it. I'll give it more thought...

For now, I will create my own pbastowski:ng-babel package, which processes .es6 files and ng-annotates them all in one. I want to publish it to Atmosphere for others to use, but I have to work out how to publish Meteor packages first.

@pbastowski
Copy link
Contributor

My angular-babel package is on Atmosphere. Look for pbastowski:angular-babel. My first Meteor package :) Mostly ripped off grigio:babel, though.

@netanelgilad
Copy link
Contributor

Awesome @pbastowski.
You should PR the README and add your package to the additional packages.
🍻

@Urigo
Copy link
Owner

Urigo commented Apr 14, 2015

👍 so good!

@jasperkuperus
Copy link

For those using mquandalle:harmony and also in need of ngAnnotate, check out: jasperkuperus:angular-harmony.

As my project heavily relies on ES6 export, I wasn't able to use angular-babel of @pbastowski. Therefore, inspired by @pbastowski, I made a fork of mquandalle:harmony that adds the extra ngAnnotate step.

@pbastowski
Copy link
Contributor

Hi @jasperkuperus. If you ever want to try Babel, I have a Meteor module pbastowski:require that I use with pbastowski:angular-babel to do such things as:

defectModel.es6.js

export class defectModel { ... }

and in defect.es6.js

import defectModel from 'defectModel';
class defect extends defectModel { ... }

@jasperkuperus
Copy link

@pbastowski thanks! I might try that one day! :)

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.

6 participants