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

Add ES6 Transpiler? #1812

Closed
heff opened this issue Jan 19, 2015 · 22 comments
Closed

Add ES6 Transpiler? #1812

heff opened this issue Jan 19, 2015 · 22 comments

Comments

@heff
Copy link
Member

heff commented Jan 19, 2015

With the addition of browserify we're adding a required build step for running the code (i.e. can't just load the source files directly into the browser). Without going overboard, we could also consider adding other required build steps, like an ES6 transpiler.

A benefit would be that we could write more modern javascript today. A downside would be that it's yet another layer of abstraction adding potential complication to the debugging process.

Any other positives/negatives/comments/thoughts?

As a side note, the addition of ES6 would add the use of let, which would be a good point to re-evalute our single-var vs. multiple-var style rule. See #1781.

@dmlap
Copy link
Member

dmlap commented Jan 23, 2015

I'm torn on this one.

@mmcc
Copy link
Member

mmcc commented Feb 27, 2015

Just to throw my 2-cents in, I like this idea. It would allow us to clean up so much code using string interpolation alone (like I mentioned in #357).

@gkatsev
Copy link
Member

gkatsev commented Feb 27, 2015

@mmcc
Copy link
Member

mmcc commented Feb 27, 2015

👍

@dmlap
Copy link
Member

dmlap commented Feb 28, 2015

If we go this route (and I'm in the "yes" camp these days, too), I think we should stick to ES6 module syntax and not adopt any browserify/node-isms. Browserify is great but sticking to language features will give us more flexibility if something even more magical pops up in the next year or so.

@gkatsev
Copy link
Member

gkatsev commented Feb 28, 2015

I do think that browserify is still the way to go right now. Whether we use the ES6 module syntax or not.
The main reason why I think browserify is the way to go is because of npm. There are tons of modules there that we could depend on and thus allow us to maintain less code ourselves (xhr for example).
In addition, babel seems compile ES6 module syntax to commonjs and I dont think that jspm and SystemJS (as cool as they are) are quite ready for full-on production. Particularly because they have a much larger overhead at runtime than a browserify-built file.
And with babelify, we can use the ES6 module syntax if we really wish. Though, I still think we should stick with commonjs syntax.

@gkatsev
Copy link
Member

gkatsev commented Feb 28, 2015

Case-in point: https://github.com/videojs/video.js/pull/1909/files we wouldn't need to implement or maintain it if we could've just grabbed it from npm/browserifiy.

@mmcc
Copy link
Member

mmcc commented Mar 2, 2015

I don't see these things as being mutually exclusive (so I think I mostly agree with Gary). My preference would be to use ES6 modules for internal requires, and standard require statements for things coming from NPM. Under the hood, as you said, all these things are converted to Commonjs by Babel anyway, so there isn't any performance penalty there.

If we're going to go with ES6 syntax and start transpiling, why would we stop at Commonjs syntax? Going "all in" on ES6 for us would simply mean using a Babel transform in our Browserify step, which would functionally change nothing, but (theoretically) put us in a better position in the future.

@gkatsev
Copy link
Member

gkatsev commented Mar 2, 2015

I think it's weird to combine node-requires and ES6-modules. Given that babel compiles it node-requires, I don't see a huge benefit from using ES6-modules yet if we're going to be using node-requires as well.
If we can require npm modules via ES6-modules, then I'm ok with using ES6-modules.

@mmcc
Copy link
Member

mmcc commented Mar 2, 2015

We would need to do a spike to confirm everything works as expected, but based on what I'm reading in Babel's docs, we can use import syntax to simply require npm modules as well.

@gkatsev
Copy link
Member

gkatsev commented Mar 2, 2015

Sounds like we can do it with the module formatters which has built-in support for commonjs.

@heff
Copy link
Member Author

heff commented Mar 5, 2015

It sounds like everyone's on board with this, yes? @mmcc is going to do a research spike to see what kind of IE8 issues we may run into since it compiles to es5.

@heff heff added confirmed and removed needs: discussion Needs a broader discussion labels Mar 5, 2015
@gkatsev
Copy link
Member

gkatsev commented Mar 5, 2015

We should make es5-shim a dependency. Either by including it ourselves directly or by requiring users of videojs to load in es5-shim before videojs.

@mmcc
Copy link
Member

mmcc commented Mar 6, 2015

Ok so I've done a small spike to figure out what this would mean for us in practice.

  • If we want to use classes, we'll need both the es5-shim and the es5-sham.
  • Running Babel on combined.video.js (with no changes to the code base) does not negatively affect IE8 at all.

So, with those two things in mind, if we want to actually do this, I think step 1 is to add Browserify with the Babelify transform and go through and re-do the Browserification work that way. Once we're done with that, we can start to experiment with ES6 syntax in other areas of the code base and see where IE8 starts barfing.

Along with all of this, I think this starts to call for an optional IE8 import. Basically, something in the readme along these lines:

If you want to support IE8, you'll need to include IE8-specific Javascript and CSS.

<!--[if IE 5]>
  <link rel="stylesheet" type="text/css" href="ie5.css" />
  <script src="video.ie8.js"></script>
<![endif]-->

@gkatsev
Copy link
Member

gkatsev commented Mar 6, 2015

Classes like the new class syntax?

@mmcc
Copy link
Member

mmcc commented Mar 6, 2015

Yeah, it would feel pretty good to be able to do stuff like this:

class Component extend CoreObject {
  constructor(player, options, ready) {
     /* ... */
  }
}

@gkatsev
Copy link
Member

gkatsev commented Mar 7, 2015

@mmcc
Copy link
Member

mmcc commented Mar 7, 2015

Yes?

@gkatsev
Copy link
Member

gkatsev commented Mar 7, 2015

Just noting it. Nothing new, specifically. Though, they do want you to use their polyfill along with es5-shim. I wonder what exactly it adds compared to es5-sham.

@gkatsev
Copy link
Member

gkatsev commented Mar 7, 2015

Also, it's possible that we don't need es5-sham if we tell babel we want "loose mode" for classes and maybe even for modules too.

@heff
Copy link
Member Author

heff commented Mar 7, 2015

Yeah, sounds like we just need to use loose mode (which doesn't sound bad) and not use new features of the classes yet (static/getters/setters), but even without those we'd at least be using a standard class implementation instead of our own implementation.

If we don't need the sham that'd be great.

@mmcc mmcc added this to the v5.0.0 milestone Apr 1, 2015
@heff heff closed this as completed Apr 25, 2015
@heff
Copy link
Member Author

heff commented Apr 25, 2015

Closed by #1976

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants