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

Support ast build handlers #51

Open
raix opened this Issue Dec 19, 2014 · 17 comments

Comments

Projects
None yet
6 participants
@raix
Copy link
Contributor

raix commented Dec 19, 2014

Why? - Let packages modify and improve code in steps,

Examples of packages that could do work on the code:

  • Repporting tools like jshint etc. non intrusive
  • Minifiers / Optimizers - Modifiers

The quick fix just lets multiple source handlers look at the source - basicly to allow code reporting / jshint

Your use case could bring a really nice / important improvement - having eg. a source handler for replacing Meteor.isClient and Meteor.isServer with true/ false would be nice to run before eg. uglify (uglify would then remove the dead code/unused code blocks)

I'm not sure of a way to let the bundler sort source handlers - first thought was a unix like load levels init 1 - 5 each level runs source handlers unordered - but ordered to levels - but the levels would be triggy to define / standardize?

An other issue is that:

   Package.register_extension("js", jshintHandler);

jshintHandler is not handed the actual source?

var jshintHandler = function(bundle, sourcePath, serverPath, where) {

Could do something like:

   Package.register_extension("js", jshintHandler, initLevel);

where initLevel is some sortable level

  // Allowing packages to modify the AST
  var jshintHandler = function(bundle, sourcePath, serverPath, where) {
  bundle.source === theManHandledSourceSoFar;
  bundle.source = theNewUglifyedSource;

Something like that could be nice - but it would require some work - and extruding the uglify to a package - but this would be an improvement too?

Quotes taken out of context - read ref links to get it all:

@raix:

I wrote a Meteor-jshint package,
To run jshint on .js files is currently not allowed since there can't be 2 or more source handlers.
A source handler doesn't nessesarly change the code - Like in the jshint case - its just parsing and checking the code and reporting in on bad gramma.
I kept the warning about "two packages ..." - it only fires once pr. extension to keep down the verbosity

@gadicc:

I'd like to add my support to this and provide another use case:
The messageformat smart package currently extracts strings for translation from files using a CLI tool installed via npm. It would be super cool to rather do this via source handlers, making the process completely integrated and invisible to the developer.

@mquandalle:

Yes this is an idea that will really push the limit of the packages system further. I have a lot of ideas of packages that would benefit to be able to access or even modify the JavaScript code.
This is a must-have for Meteor 1.0.

@ccorcos:

I totally agree!
I think the biggest issue here is that source handlers go into an object. So the reason you can only have on source handler is the key will get overwritten. Seem like it should be an array as opposed to an object ;)
In any case, I am looking for this feature as well so I can compile HTML, JS and CSS all from the same file! How amazing does that sound ;)

@glasser:

Actually this sounds like a reasonable enough idea. I am looking into implementing it with the new bundler.
Agreed that package.js needs refactoring. The 0.6.5 "linker" release was originally going to involve a rewrite of package.js, but we've decided that in the interest of ever releasing it to push that to future work. But it does have a new (non-user-visible) package manifest file in the new built "unipackage" format.

Ref:

@mquandalle

This comment has been minimized.

Copy link

mquandalle commented Dec 19, 2014

I've been trying to implement this functionality in the meteor tools, and actually my opinion has changed since our discussion last year in meteor/meteor#1207.

I think it's a good model to have one source handler per extension. Basically we are using it to transform any language (html, coffee, coffee.md, jade, harmony, etc.) into [html]/css/javascript.

What would be useful I think is a way to take a complete compiled “image” (for instance the browser application) and modify it (for instance to remove Meteor.isServer code, or to do some minification, or any other transformation). The idea here is that we don't handle one particular file but all the code of a compiled application/target/image. This handler would work with the AST, so we would have to parse and re-compile the code only once (which IIRC is currently done by js-analyse package anyway, so we don't have more performance penalty by doing that).

Modifying the AST might not be the most simple task for package authors, but here come the excellent recast library by @benjamn (who is currently working at MDG).

The API could be something like Plugin.registerImageRecaster, to be consistent with the Plugin.registerSourceHandler. But actually I think the world register doesn't bring much in this context, so I would prefer to have something like:

  • Plugin.handleSource(extName, [options], handler)
  • Plugin.registerCommand(name, [options], action) #9
  • Plugin.recastImage([options], recaster)

In the recastImage method, the optional options parameter is used to specify an archMatching so we could target a particular image, and also to specify a priority (between 0 and 1) so we could sort recasters (as we already discussed in meteor/meteor#1207 replacing Meteor.isServer by false on the server must be done before removing the dead code).

Open questions:

  • Should we do something similar for CSS?
  • Do we really parse/transform/re-compile/ a whole image everytime the developer modify a single file? Could we do some incremental compilation (only sending parts of the AST that has changed)? Or maybe the solution is to allow a developer to create multiple images/sub-applications from the same source code?
  • Do we provide the recast lib to the handler, or just the AST and then the plugin author is free to use whatever lib he wants to modify this AST?
@benjamn

This comment has been minimized.

Copy link

benjamn commented Dec 19, 2014

One convenient property of Recast is that it doesn't care how you do the AST transformation, so I think it makes a lot of sense to give the plugin author the AST and let her decide how to modify it. If the author decides to use recast.visit, that's great (since I can provide better support for that), but any function that modifies the AST will work!

On the implementation side, we can certainly use recast.parse and recast.print before and after the transformation, but that's an implementation detail.

@raix

This comment has been minimized.

Copy link
Contributor

raix commented Dec 19, 2014

@mquandalle Se see your point - Lets keep sourcehandlers be simple converters, one pr. file ext.
I'll update the title and have it something like a build plugin:

  • astHandlers - handling / modifying / watching the js ast

This concept would be really powerful if html/css is also converted into js format?

@raix raix changed the title Support multiple sourcehandlers for js Support ast build handlers Dec 19, 2014

@raix

This comment has been minimized.

Copy link
Contributor

raix commented Dec 19, 2014

btw. @benjamn I read the recast code awhile back - I kinda liked the concept - It would be nice to use it in the build tool, would allow us to do crazy stuff

@mquandalle

This comment has been minimized.

Copy link

mquandalle commented Dec 19, 2014

@benjamn, would you consider adding a package.js to recast? (#14). Then we could directly using the isopack loading method from the meteor core tools without the need to add one more npm dependency in the dev bundle.

@mquandalle

This comment has been minimized.

Copy link

mquandalle commented Dec 23, 2014

@raix, I guess you would be able to rewrite https://github.com/raix/Meteor-famono require lookup using the AST instead of doing black magic on the client side; is that what you have in mind?

@raix

This comment has been minimized.

Copy link
Contributor

raix commented Dec 23, 2014

@mquandalle Famono will deprecate if this became a feature - we could do alot of code optimizations eg. removing dead code and merge alike functions etc. This way we wont have to think about including the whole famous library.

But I've got a lot of other ideas for this feature too,

@benjamn

This comment has been minimized.

Copy link

benjamn commented Dec 23, 2014

@mquandalle definitely happy to add a package.js file (but also happy to take a PR if someone beats me to it)!

@raix

This comment has been minimized.

Copy link
Contributor

raix commented Feb 17, 2015

It seems as if facebook is creating a system much like meteor - but with some very important improvements, many of those are sadly old feature requests to meteor that haven't been touched yet.
They already support ast build handlers as a feature.

@mitar

This comment has been minimized.

Copy link

mitar commented Feb 17, 2015

Which system are you talking about?

@raix

This comment has been minimized.

Copy link
Contributor

raix commented Feb 17, 2015

The Facebook stack:

  • React - Blaze
  • React router - Like iron router, but not quite..
  • React native - Cordova (except its native)
  • GraphQL - Subscriptions / minimongo (but ensures that you dont over/under subscribe data)
  • Relay - Live Data / Latency compencation (not sure if its actually cached / offline)
  • Webpack - Isopack / Build Tool

This FR is the type "Loaders"AST transformations in webpack

https://www.youtube.com/playlist?list=PLb0IAmt7-GS1cbw4qonlQztYV1TAW0sCr

@mitar

This comment has been minimized.

Copy link

mitar commented Feb 17, 2015

But for server side they do not provide anything? Node? PHP?

@steph643

This comment has been minimized.

Copy link

steph643 commented Feb 17, 2015

Just to connect the dots, here is the discussion about Meteor and React (started by @mitar):
https://groups.google.com/forum/#!topic/meteor-talk/crD-tGGLDPY

@raix

This comment has been minimized.

Copy link
Contributor

raix commented Feb 17, 2015

I'm not sure @mitar they mention it in the end of the Relay talk I think

Thanks @steph643 for the reference (I'm not subscribing to the meteor talk due to spam) additional ref: meteor/meteor#3728

@mitar

This comment has been minimized.

@mquandalle

This comment has been minimized.

Copy link

mquandalle commented Apr 20, 2015

The stream based build tool proposal is an exciting architecture proposal! It seems that Babel transformers could be used to handle AST modifications, see https://speakerdeck.com/sebmck/babel-facebook-talk.

@ccorcos

This comment has been minimized.

Copy link

ccorcos commented Apr 20, 2015

That would be awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment