add runtime option

[guzart]

add a runtime option that includes the traceur runtime file, references #23 and #5

[thlorenz]

Thanks for taking a stab at this, but as far as I understand this you'd prepend the runtime for every file.
That is not an option.

[guzart]

• The runtime is only prepended to the first file that is processed.
• The main method acts as a pure transform and as a pure transform factory.
• It is true that I did not implement the options as you suggested. It' because I dont undertand the need to nest the traceur options, I dont think the property names will clash, and it just makes the option object more complex.

Have you tested the PR, because it's all working for me.

[thlorenz]

Let me have another look later, but maybe in the meantime you could add some tests to show that and how this works?
That would make it easier for me to understand what you are doing and make a decision about how to pull this change in.

As far as I can see the only thing wrt traceur runtime that changes is that we pass a boolean flag to have it prepended instead of explicitly adding it to browserify. I'm not even sure if that is that much of an improvement since sometimes explicitness is preferred.

However once I see some tests we can think about which one makes more sense. At any rate the .add(es6ify.traceurRuntime) option needs to stay around either way for backwards compat.

So keep in mind that at this point I'm somewhat on the fence about this feature as I'm not convinced that it is a better API even though it is more terse.
Therefore just be aware that I don't promise anything even if you add some tests. Just wanted to throw that out before you invest more time.

However I'm more open to just adding another way to specify traceurOverrides via es6ify.configure.
Thanks

[guzart]

This PR's were just to start the conversation. However, the PR's are backward compatible with:

• es6ify.traceurOverrides = { blockBinding: true }
• .add(es6ify.traceurRuntime)
• .transform(es6ify)
• es6ify.configure(/\.js$/)

And they enhance the functionality with:

• .transform(es6ify({ blockBinding: true })
• .transform(es6ify({ runtime: true })
• .transform(es6ify({ filePattern: /\.js$/ })

I am also on the fence about the terse options, I guess I do prefer the traceur options being explicit. So I'll spend some time tonight refactoring towards:

.transform(es6ify({
  traceurRuntime: true,
  filePattern: /\.js$/,
  traceurOverrides: { blockBinding: true }
}))

What do you think? (Of course, w/ backward compat)

Edit: Now that I look at it, traceurRuntime: true doesn't tell me much, maybe addTraceurRuntime: true.

[guzart]

BTW, this change also allows this usage:

browserify('./app/main.js')
  .transform(es6ify({ addRuntime: true }))

as opposed to having to add the entry file using the .require() function.

browserify()
  .add(es6ify.runtime)
  .transform(es6ify)
  .require(require.resolve('./app/main.js'), { entry: true })

[thlorenz]

Actually this is due to a change in browserify (the examples are old), but has nothing to do with your changes IMO.

[thlorenz]

will have a closer look later today, thanks.

[guzart]

I am using "browserify": "^3.38.0", and this adds the runtime:

browserify()
  .add(es6ify.runtime)
  .transform(es6ify)
  .require(require.resolve('./app/main.js'), { entry: true })
  .bundle({ debug: true })
  .pipe(fs.createWriteStream(bundlePath));

But this does not:

browserify('./app/main.js')
  .add(es6ify.runtime)
  .transform(es6ify)
  .bundle({ debug: true })
  .pipe(fs.createWriteStream(bundlePath));

With traceurify I was specifying browserify entry point using the latter. But maybe am missing something.

[thlorenz]

Ah, that is actually quite odd, but I guess you are right.
This is gonna be hard if we want to preserve backwards compat.

I like some of these ideas however, so we could also say just screw it lets overhaul the entire API without staying backwards compat and publish as a major release. The requirement to stay useful as pure transform remains however.

In that case I'd also opt for including the traceur runtime by default since that makes it easier for newcomers as every feature would work out of the box.

Disabling that could be an advanced feature.

I'd like to see some examples that show how to use that new API (at this point forget about backwards compat). You don't have to implement it either.
Just add some code to the examples dir that shows how you'd like to interact with es6ify.

We should also think about how to configure es6ify from the package.json so you could do browserify -t es6ify and the options get picked up.
Have a look at browserify-shim to get an idea what I mean.

Extra bonus for sticking with this despite me being so conservative about these changes. :)

Thanks.

[guzart]

I feel like I've always left the plugin with the "pure transform" functionality intact, but maybe we have a different idea of what "pure transform" means.

For me a "pure transform" means a function that takes a file path as parameter and returns a Stream.

function(file: string): Stream

[thlorenz]

I feel like I've always left the plugin with the "pure transform" functionality intact

I believe you did, I may have misinterpreted what you did previously, but as long as es6ify is module.exports = function (file:string) : TransformStream then all is good.

[guzart]

hey @thlorenz, I've modified the example to look more like the api I suggested before, and refactored the existing code just to enough to support it. build.js

1. Pass the options directly into the es6ify function, it's simple and straightforward, with support for browserify from the console. build.js@12
2. Add the traceur runtime by default but allow to disable with addTraceurRuntime: false option build.js@13. I went with the add prefix, because that's what traceur uses internally traceur-compiler-options
3. Set the traceur options by using the traceurOptions: {} option. build.js@14, also decided to use the Options suffix because traceur also calls them options.

I think everything is still backwards compatible

[guzart]

Thank you for being open to my suggestions and for your feedback

[thlorenz]

This is great, keep 'em coming!

I'll create a separate branch tomorow to allow us to play with these ideas.
We can take our time with experimenting as a major version was just released and we should come up with the best possible API.

[thlorenz]

I created a branch with your changes and added you as collaborator.

Feel free to experiment to your delight there, just please don't merge anything into master.
Instead once we figure out how to do this exactly, we'll finalize this on that (or another) branch and I'll merge into master and publish a new version.

Thanks.

[thlorenz]

As far as I can see you did some nice work making the API smart enough to work with either options or just a file.
The main challenge will be to document this clearly.

So configure and traceurOverrides don't need to be exported anymore.

As I can see we'll just expose one function, es6ify. We need to merge the docs/comments from all the functions we remove to explain all the things es6ify can do.

The only additional function we'll expose is compile to use for library authors.

decided to use the Options suffix because traceur also calls them options

The point is that you are just overriding default options vs. setting them. I picked the name to make that clear and would like to keep it that way.

[thlorenz]

Actually for now instead of deleting v1 functions lets just keep them but print a warning that they are obsolete whenever they are used.
We could make traceurOverrides a __defineSetter__ in order to do that.

Also we'd need to add some tests that use the new API and move the current ones into transform-compat.js and bundle-compat.js.

So lots of work, but we got time :)

[guzart]

Thank you for adding me as a collaborator.

So I think this is what's on the v2 agenda:

• Move current tests to transform-compat.js and bundle-compat.js
• Print obsolete warning when using v1 functions.
• Update the es6ify API
  • Add tests for the v2 es6ify [in progress]
  • Support the following options:
    • filePattern: RegExp = /\.js$/ file pattern for processable files
    • addTraceurRuntime: boolean = true whether to include the traceur runtime
    • traceurOverrides: Object = {} traceur options
• Configure es6ify from the package.json so you could use the console command browserify -t es6ify
• Ensure docs are up to date

Are you ok with creating separate issues for each item?

[thlorenz]

Let's keep the discussion central.

We'll just add commits that take on each task an put enough comments in order to not even need too much discussion.
Feel free to make lots of small commits to enable lots of discussion, we'll squash them later.

[thlorenz]

Haven't heard about this in a while.
Have you made any progress or is any action required on my part before you can continue?

Thanks.

[guzart]

Sorry @thlorenz I've been busy with other projects, I will spend some time later this week to work the bullet points.

In terms of the API, I think am happy with this, but I'll setup the examples and the tests

browserify('./app/main.js')
  .transform(es6ify({
    filePattern: /\.js$/,
    addTraceurRuntime: true,
    traceurOverrides: { blockBinding: true }
  }))

Cheers,

[thlorenz]

No worries, this is not urgent at all, so take your time and make it pretty :)

[guzart]

Updated the "agenda comment"

I think I finally understand why the traceurOverrides. Currently, you can set the traceur options once for the imported es6ify function and then all the subsequent calls will use those options, e.g.

var es6ify = require('es6ify');
es6ify.traceurOverrides = { blockBinding: true };
browserify().transform(es6ify);  // uses blockBinding = true
browserify().transform(es6ify);  // uses blockBinding = true as well

The reason I wasn not thinking of overrides, is because on my PR you might pass the options the first time but next time, you get the defaults.

var es6ify = require('es6ify');
// uses blockBinding = true
browserify().transform(es6ify({ traceurOptions: { blockBinding: true }}));
// uses default blockBinding = false
browserify().transform(es6ify);   

I think we could keep both for maximum flexibility, have the traceur overrides the way it is currently implemented, and allow to pass specific options for each transform. Tho it might get more complicated to explain, understand the difference on which one to use.

[thom4parisot]

Do you have any update on that feature? I was surprised not to be able to use the -t es6ify in the CLI.

[thlorenz]

@oncletom this has been in the works for a while now.
We were trying to come up with a really solid API for version 2.0.

I suppose we should start focusing on this again @guzart and also get @domenic's opinion on some of this.

We should also consider turning es6ify into a plugin. This has been done quite successfully for proxyquireify.

[thom4parisot]

Cool thank you :-)