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 babelify support #332

Merged
merged 1 commit into from
Nov 22, 2017
Merged

add babelify support #332

merged 1 commit into from
Nov 22, 2017

Conversation

yoshuawuyts
Copy link
Member

Adds Babelify support. Went with a config that's hopefully reasonable-ish. Breaks some of our modules (e.g. choojs/choo-devtools#20) but nothing we can't fix. Hope this good; thanks!

@Flet
Copy link
Collaborator

Flet commented Nov 14, 2017

Functionally sound for me! Tested on a project that was wired up to use babelify+yo-yoify and it works without issue. 👍

@toddself
Copy link
Contributor

Babelify might not be a great option to force upon users, especially with a global: true flag, as there are a lot of issues that can arise from using code that becomes "difficult" to "babelize".

The file size issue for including polyfills really depends on how much ES6 you're using and what versions you're looking to support. Safari 9, for example, supports very little ES6, but is also very old and low on marketshare (much like IE 11). A Safari 10 bundle is smaller, and Safari 11 is even smaller...

However, due to how some of the babel transforms work, forcing babelify on bundles creates some extremely undesirable outcomes.

All file sizes obtained via du -h *

choo.js

bankai#babelify:

Command: git clone [email protected]/choojs/choo && cd choo && npm i && npm i choojs/bankai#babelify && node_modules/.bin/bankai build .

file size
uncompressed 16k
brotli 4.0k
deflate 8.0k
gzip 8.0k

bankai#9.0.0-rc10 (babelify target for safari >= 9)

Command: git clone [email protected]/choojs/choo && cd choo && npm i && npm i [email protected] && node_modules/.bin/bankai build .

file size
uncompressed 12k
brotli 4.0k
deflate 8.0k
gzip 8.0k

db-worker-store (uses es6 classes, async/await, fetch, Symbol, etc and requires babel-polyfill for async/await)

bankai#babelify

file size
uncompressed 120k
brotli 36.0k
deflate 40.0k
gzip 40.0k

bankai#9.0.0-rc10 (babeilfy target for safari >= 9)

file size
uncompressed 120k
brotli 36.0k
deflate 40.0k
gzip 40.0k

bankai#9.0.0-rc10 (no babelify, no babel-polyfill)

file size
uncompressed 36k
brotli 12k
deflate 12k
gzip 12k

What the above illustrates though is the real problem. If I'm using any feature that requires the use of the rengeneratorRuntime for babel to properly transcode down to a specific target (async/await is the main culprit here, but there are other generator-based transforms), I have to include babel-polyfill in my project (or the regeneratorRuntime itself, but it in turn relies on a fully-formed ES6 runtime, so you need babel-polyfill to support IE 11 as it stands).

Being that async/await has been supported since Chrome 55 (Nov 2016), Firefox 52 (Mar 2017), Safari 10.1 (Mar 2017) / iOS 10.3 (Mar 2017), Edge 15 (Apr 2017), Opera 42 (Dec 2016), I might want to transform this code at all. However, if I use bankai I now have to transform my code. This results in an error even in a browser that supports async/await:

image

Even if the file size was negligible (which at 36-38k compressed my opinion is that its not) it still forces users to know that they need the runtime, and the onus is on them to include it..

@toddself
Copy link
Contributor

Note: the use of generators in your code with a target that includes IE 11 will also require the regeneratorRuntime (and any other code that it requires to support)

@toddself
Copy link
Contributor

Dropping IE 11 and the 1% support would probably be for the best. It won't transpile generators, but we should specifically call out async/await support in the README & Docs.

I am using our current browser matrix though as a basis for this. Since we deal with larger enterprise-type media companies who tend to be very slow in upgrading, we see Chrome/Firefox on Win 7 as our oldest PC target (we specifically do not support IE 11 for those customers though), and Safari 10 on OS X 10.10 is the oldest target we see on the mac side of the equation.

@Flet
Copy link
Collaborator

Flet commented Nov 14, 2017

Defining target browsers feels subjective and will absolutely change over time. I can see this turning into lots of issues/requests to add/drop support for different browsers based on each projects target audience...

Would it make sense to use some sensible default and allow folks to override it via config? This could allow for folks to get started with bankai quickly while letting others tweak the output to match their use case.

maybe add a bankai key to package.json to override the default?

"bankai": {
  "targets": {
   "browser": ['last 2 versions']
  }
 }

@Flet
Copy link
Collaborator

Flet commented Nov 14, 2017

I have to include babel-polyfill in my project

I wonder if this is a bug in babel-preset-env? I thought it was supposed to do this for us? :)

@yoshuawuyts
Copy link
Member Author

@Flet I think it's supposed to be something like this: https://www.npmjs.com/package/babel-preset-env#usebuiltins

@Flet
Copy link
Collaborator

Flet commented Nov 15, 2017

Ah ok. Nothing is as magical as I hope :(

@yoshuawuyts
Copy link
Member Author

Alright, so it looks to support newer platform features you need to import babel-polyfill. This is never done by default, and needs to be done explicitly. The useBuiltIns flag helps somewhat, but it still includes things like regenerator and the like which are rather large.

The polyfill itself is around 20kb, even if you don't use any of it.

I think the best option for now would be:

  • by default support IE11
  • by default don't pull in babel-polyfill (until there's better feature detection)
  • allow extending using .babelrc (until there's better feature detection)

I think that would be the path of least resistance, for now. Thoughts?

@toddself
Copy link
Contributor

toddself commented Nov 16, 2017

I agree with points 2 & 3 :)

The only issue with IE 11 is that people might not consider generators to be a new platform feature (since they've been available in Chrome & Firefox platforms since at least 2014l; even Edge has had them since late 2015).

I am a strong proponent of dropping IE 11 support from everything being that it's been end-of-lifed, has really bad support for a lot of CSS (flexbox has so many obnoxious little bugs) and will never get a release or an update again.

Even MS recommends that IE 11 users not use it post Jan 2016 because other vendors are likely not going to support it.[1]. Support for IE 11 on Windows on Windows 7 is over, Windows 8 support for IE 11 ends in January 2018, and then 2020 for Windows 10.

Given this and that IE usage is well under 5%[2] and trending very rapidly downwards[3]. I would recommend we set last 2 versions, not ie > 8 since last 2 versions considers IE 11 and 10 to be "versions" and I don't think we're going to consider adding IE 10 a good idea?:

Features not supported by IE 11, but supported by last 2 browsers that require a lot of extra bytes when transcoded:

  • ES6 Class syntax [99b for the ES 6 syntax, 1,204b for the transpiled version)[4]
  • ES6 Generators [32b for ES6, 382b for transpiled version. This has the additional requirement of babel-polyfill which will only surface as runtime error][5]

If someone wants to support IE 11, they can opt-in by specifying it in their own .babelrc file and take the hit themselves, but I feel like that's a lot of junk to support an old browser, that is going away.

4

ES6 Classes


ES 6 Syntax

class Bar {
  constructor () {
  }
}

class Foo extends Bar {
  constructor () {
    super()
  }
}

ES 5 transpiled

"use strict";

function _possibleConstructorReturn(self, call) { if (!self) { throw new ReferenceError("this hasn't been initialised - super() hasn't been called"); } return call && (typeof call === "object" || typeof call === "function") ? call : self; }

function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function, not " + typeof superClass); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, enumerable: false, writable: true, configurable: true } }); if (superClass) Object.setPrototypeOf ? Object.setPrototypeOf(subClass, superClass) : subClass.__proto__ = superClass; }

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

var Bar = function Bar() {
  _classCallCheck(this, Bar);
};

var Foo = function (_Bar) {
  _inherits(Foo, _Bar);

  function Foo() {
    _classCallCheck(this, Foo);

    return _possibleConstructorReturn(this, (Foo.__proto__ || Object.getPrototypeOf(Foo)).call(this));
  }

  return Foo;
}(Bar);

5

ES6 Generators


ES 6 Syntax

function* bar (x) {
  yield x
}

ES 5 transpiled

"use strict";

var _marked = [bar].map(regeneratorRuntime.mark);

function bar(x) {
  return regeneratorRuntime.wrap(function bar$(_context) {
    while (1) {
      switch (_context.prev = _context.next) {
        case 0:
          _context.next = 2;
          return x;

        case 2:
        case "end":
          return _context.stop();
      }
    }
  }, _marked[0], this);
}

@yoshuawuyts
Copy link
Member Author

I am a strong proponent of dropping IE 11 support from everything being that it's been end-of-lifed

I think you might be confusing it with IE10; IE11 is not due for end of life until 2025 (source).

ll under 5%[2] and trending very rapidly downwards[3]. I would recommend we set last 2 versions, not ie > 8 since last 2 versions considers IE 11 and 10 to be "versions" and I don't think we're going to consider adding IE 10 a good idea?:

At the moment IE11 is the only MS browser on Windows 7, which is still widely used. I have no numbers on this, but I've got a hunch that people using Windows 7 + a built-in browser are probably either not tech savvy, or have limitations imposed from the outside.

Think if folks are building civic tech; discarding 1 in 20 people is significant. I think the default for general purpose tools should be to not exclude anyone (within reasonable margin). If people want to exclude browsers, it's up to them.

and trending very rapidly downwards[3]

This is great; once it drops low enough I agree we should drop it. Just not quite yet. If you want to get ahead of the stats, we should allow the option to edit .babelrc so people can pick their own browser support.

README.md Outdated
@@ -70,6 +70,7 @@ can be useful to know which optimizations we apply. This is a list:
useful in combination with minification, which removes unused code paths.
- __brfs:__ Statically inline calls to `fs.readFile()`. Useful to ship assets
in the browser.
- __babelify:__ Bring the latest browser features to _all_ browsers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we (should we) put a note about the regeneratorRuntime/babel-polyfill for IE 11 users if you're using async/await or generator functions?

It's super hard to find good information about this online (there's a few github tickets on non-babel repos that explain it)

@yoshuawuyts
Copy link
Member Author

Merging! — gonna release this as a quick RC first, and then major after. Yayay!

@yoshuawuyts yoshuawuyts merged commit c45c23f into master Nov 22, 2017
@yoshuawuyts yoshuawuyts deleted the babelify branch November 22, 2017 14:14
@YerkoPalma
Copy link
Member

Wow! Are all issues with bankai fixed? Last thing I knew about dynamic routes is that they were broken for build command because of ssr...
Despite of other minor issues, that's critical, for me at least 😕

@yoshuawuyts
Copy link
Member Author

RC 11 out, v9 stable later today

@yoshuawuyts
Copy link
Member Author

Last thing I knew about dynamic routes is that they were broken for build command because of ssr

Yeah, those don't work, but that's by design. You can't output a dynamic route, if you don't know what data is being passed to it. So it needs to happen on the fly, unfortunately. The way to work around it for static sites is by doing what we did here: https://github.com/yoshuawuyts/millennial-js/blob/master/index.js

We'll need to figure out a way to allow async rendering from within Bankai though; and that's tied to async rendering from within Choo. I'm estimating that's at least a month out, so it would make sense to release Bankai v9 now, and add it once we've figured it out. Does that make sense?

@YerkoPalma
Copy link
Member

Mmm I don't want to be rude or mean but if bankai isn't handling dynamic routes, is actually breaking my apps... I guess I'll open a specific issue for build on dynamic routes, I have some thoughs on posible workarounds/fixes.

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Nov 22, 2017

@YerkoPalma hey, yeah no worries — so we are handling dynamic routes, however we can't compile them to disk. We can't do that, because it's impossible to know what data to use during compilation (that's the dynamic part).

Before we proceed, it's worth pointing out that none of this applies for static sites. Everything will work as expected.

If you're using dynamic routes, you'll probably want to use bankai/http. When data comes in, you handle the request, and new templates pop out on the other side. You can combine this with bankai build to warm up the CDN with all static assets. Any requests that then aren't handled, fall back to bankai/http.

This all works in Bankai, today. Now the part that we still need to figure out in Choo, is how to do async rendering. Once we figure that out, we can integrate it into Bankai. This is useful to prefetch data, server-side.

Does that make sense?

@YerkoPalma
Copy link
Member

@yoshuawuyts opened #336 so other people could get track of discussion.

Agree on the static sites point, but my main concern is that this feature was supported previously by bankai.

If you're using dynamic routes, you'll probably want to use bankai/http

That if I want to have ssr, for me a working site is more important than a ssr site. I basically want that after running bankai build and dropping the output folder to my gh-pages, netlify, or whatever service for spa I'm using, it works, like before.

@yoshuawuyts
Copy link
Member Author

@YerkoPalma hold on; are you saying that dist/:foo/bar/index.html would be picked up by netlify?

@yoshuawuyts
Copy link
Member Author

@YerkoPalma oh, also what about dist/foo/:bar/*/index.html ? IIRC you can't create a directory named '*' :(

@YerkoPalma
Copy link
Member

@YerkoPalma hold on; are you saying that dist/:foo/bar/index.html would be picked up by netlify?

Well, I have my personal site https://github.com/YerkoPalma/me built on choo, hosted in gihub pages and using dynamic routes. Those are handled by choo so it works fin, not exactly this dist/:foo/bar/index.html but this dist/:foo/bar. Now, if you directly access to some dynamic route, it throws a 404, so I added a 404.html file, and now, every route is redirected and handled by my main script.

Now the obvious tradeoff is that there is no ssr.

oh, also what about dist/foo/:bar/*/index.html

Haven't tried that one :D

Oh, and in netlify, happens the same, I'm hosting this https://github.com/YerkoPalma/choo-phaser choo game with dynamic routes and is almost the same like in gh pages

@amsross amsross mentioned this pull request Nov 24, 2017
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.

4 participants