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

Upgrade build deps (Babel 7, bump webpack minor) #2036

Closed
wants to merge 8 commits into from

Conversation

johnBartos
Copy link
Collaborator

@johnBartos johnBartos commented Dec 8, 2018

This PR will...

  • Upgrade to Webpack 4 & Babel 7
  • Use harmony modules & module concatenation
  • Use @babel/preset-typescript instead of ts-loader
  • Update webpack config:
    • Fix deprecations from old webpack/babel versions
    • Remove bundle-analyzer option
    • Don't always build demo
  • Use forked version of webpack-webworkify
  • Revert changes to number-isFinite polyfill

Why is this Pull Request needed?

  • So that we can leverage the improvements in the latest versions of Webpack and Babel
  • Harmony modules with concatenation dramatically reduces size
  • Babel 7+ officially supports TS https://blogs.msdn.microsoft.com/typescript/2018/08/27/typescript-and-babel-7/
  • You can analyze the bundle without having it added as a dependency (install it globally instead)
  • It's inefficient to always build the demo; it can be rebuilt easily enough
  • webpack-webworkify supports webpack 4 only on the master branch. Unfortunately, the project looks pretty dead - I doubt it will be released. I forked it and fixed our version to its master branch.
  • It is not necessary to get the self scope for Number - it always exists in the worker context

Size diff: 224k vs 247k!

Are there any points in the code the reviewer needs to double check?

No

Resolves issues:

Chore

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@johnBartos johnBartos added this to In progress in Tooling via automation Dec 8, 2018
@johnBartos johnBartos changed the title Upgrade to webpack 4, babel 7 Upgrade build deps (Babel 7, bump webpack minor) Dec 9, 2018
@tchakabam tchakabam self-requested a review December 11, 2018 10:18
Tooling automation moved this from In progress to Changes Requested Dec 11, 2018
Copy link
Collaborator

@tchakabam tchakabam left a comment

Choose a reason for hiding this comment

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

I'm not really for returning to Babel. Not sure what others think.

@tchakabam
Copy link
Collaborator

tchakabam commented Dec 11, 2018

3k size decrease doesn't really convince me much honestly. If we chose to go for a language like Typescript I think it's better to stick with the original compiler by default. I don't see enough good strong points of Babel yet.

What do you think @tjenkinson ?

Copy link
Member

@michaelcunningham19 michaelcunningham19 left a comment

Choose a reason for hiding this comment

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

@tchakabam - looks like the file size difference is -23k assuming if @johnBartos has accurate values in the PR description 😄Which is great if that's the case.

I'm fine with whatever build tooling we use that makes sense and results in the best bundle we can release with (smaller the better).

That being said, after reading the blog post you linked, I'm worried that we're now losing the ability to do type checking as mentioned in the Caveats section:

As we mentioned above, the first thing users should be aware of is that Babel won’t perform type-checking on TypeScript code; it will only be transforming your code, and it will compile regardless of whether type errors are present. While that means Babel is free from doing things like reading .d.ts files and ensuring your types are compatible, presumably you’ll want some tool to do that, and so you’ll still need TypeScript. This can be done as a separate tsc --watch task in the background, or it can be part of a lint/CI step in your build. Luckily, with the right editor support, you’ll be able to spot most errors before you even save.

While our editors may or may not have TypeScript support so we can catch type errors during dev, there should still be a final step at the project level to do type checks.

@tchakabam @johnBartos perhaps we can find a middle ground here by adding the suggested background task mentioned in the above blog post quote^

@johnBartos
Copy link
Collaborator Author

@tchakabam 24k, not 3k. That's ~10% of the build size. I think there needs to be strong reasons not to use Babel, rather than using TSC.

@michaelcunningham19

perhaps we can find a middle ground here by adding the suggested background task mentioned in the above blog post quote^

Good idea! I can look into integrating tsc or linting. I'm leaning towards during the lint step because there's almost no ts today, and no PRs have been made with ts, so it's not efficient to always compile it

@michaelcunningham19
Copy link
Member

@johnBartos

Good idea! I can look into integrating tsc or linting. I'm leaning towards during the lint step because there's almost no ts today, and no PRs have been made with ts, so it's not efficient to always compile it

Yeah that's true, although I thought there was a way to do only type checking (no compilation/emitting) with tsc.

Perhaps --noEmit  will do that?
https://www.typescriptlang.org/docs/handbook/compiler-options.html

@tchakabam
Copy link
Collaborator

tchakabam commented Dec 14, 2018

Size diff: 224k vs 247k!

Sorry misread that obviously :) 👍 That is definitely a good reason ✅

But unfortunately that doesn't resolve some questions I have. I'll try to develop a bit on it.

I think there needs to be strong reasons not to use Babel, rather than using TSC.

Unfortunately I can't necessarily agree with that idea. I don't find it as clearly logical as you seem to.

First of all TSC is the default compiler for the TypeScript language which actually defines and implements the language-specs, it is like the defacto standard. It doesn't do minification/optimization, but it does also compile ES5+ code to ES5 and ES3. All the module bundling and optimization is imo better done further down the toolchain by chaining things via Webpack.

Babel is another compiler that now added TS support on top of it's existing support of all sorts of JavaScript features, standardized or maybe-one-day standardized, and which main purpose arised from polyfilling these modern features as they were still missing native runtime support on many platforms. Typescript however is a whole own language that is however extremely compatible with JS. Think of it a bit like C and C++ maybe. However, unlike Babel, the compiler only compiles, but never fills in any actual missing runtime libraries or native objects. That is something however Babel may still do within a TypeScript toolchain setup in order to inject certain syntax-transforms or runtime-polyfills for specific build configurations (like IE11, if we'll ever want to use modern JS features like Promises).

Now Babel can and will have features of the TypeScript language implemented later or potentially differing from the default i.e specified behavior. That is simply because unlike Babel, TSC gets developed inline by the actual specifying org of the Typescript language. Also, in Babel the error codes and compiler output will be different than what the current vast majority of the TS community is getting.

Conclusion: Compiling TS with Babel is imo experimental. The reason why I say this is simply that I am now writing a lot of Typescript, and I have been since the last 2 years, and that also implied using TSC and not Babel. Which is the case for pretty much the whole TS community I think.

Now, maybe one could also compile the TSC output with Babel (the individual modules) to leverage this binary size optimizations. Webpack can actually chain several loaders together.

What I would for example need to get convinced of is that Babel implements TypeScript support in full fledge, and also provides proper error messages. But there are actually many open questions. What is the correspondance of tsconfig.json with the Babel setup? Is that config still observed by Babel? How do error codes map?

Believe me I don't want to categorically push back on this, my concerns are objective and related to the good of the project.

I see the size decrease is important and a strong point. I would just like to have good confidence about the support of TS in Babel beforehand, and also it being well-understood how the TSC behavior and config maps to Babel.

TLDR: I believe that definitely Babel can be used for optimizing size and eventually allowing runtime compatibility or adding polyfills (if we ever want to use that), but we can actually combine it with TSC. We should use TSC to compile TS to ES5, and then run Babel on all these output JS modules + existing plain JS modules. That is a setup that can easily be achieved by chaining the loaders in webpack.

@tchakabam
Copy link
Collaborator

tchakabam commented Dec 14, 2018

I'd like to understand one thing about @babel/preset-typescript: Is Babel implementing its own TS type-checking support or is it basically calling upon tsc in the end? If that's the case I think this is fine. If not, I would push for keeping using tsc but getting the Babel optimizations by chaining it after tsc, as explained above.

Quoting from a MS blog post on the Babel support:

Using the TypeScript compiler is still the preferred way to build TypeScript. While Babel can take over compiling/transpiling – doing things like erasing your types and rewriting the newest ECMAScript features to work in older runtimes – it doesn't have type-checking built in, and still requires using TypeScript to accomplish that. So even if Babel builds successfully, you might need to check in with TypeScript to catch type errors. For that reason, we feel tsc and the tools around the compiler pipeline will still give the most integrated and consistent experience for most projects.

from https://blogs.msdn.microsoft.com/typescript/2018/08/27/typescript-and-babel-7/

Let's try to get those 10% bin size gain while not running into issues with TS on the other side :)

@johnBartos
Copy link
Collaborator Author

@itsjamie Any opinion on using TSC as a build step, but using Babel to emit the bundle?

@itsjamie
Copy link
Collaborator

itsjamie commented Feb 3, 2019

I think whichever results in a smaller build would be ideal, so Babel looks good to me 💯.

Only caveat: We would have to limit what we wrote to things that tsc could still compile, so that means no Babel stage 0/1/2 stuff. I don't think that impacts us at all, just something to note.

Took a look over the community from Babel users who use typescript. General approach seems to be right in line here. If we add a npm task to run type-checking through tsc.

npm run check-types

Which is invoked in CI, and could be run locally. Looks fine to me.

Like @michaelcunningham19 called out, no-emit would be good to enable to prevent any weirdness. pretty seems to be options folks use to make it easier to understand the output.

@itsjamie
Copy link
Collaborator

itsjamie commented Feb 3, 2019

Also, VSCode will still run type-checking as you type so long as we leave the .tsconfig file alone, and add the noEmit option to it.

"mocha": "^3.0.2",
"selenium-webdriver": "^3.1.0",
"should": "^13.1.3",
"sinon": "^4.1.3",
"ts-loader": "^4.4.2",
"ts-node": "^6.1.0",
"typescript": "^2.9.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should still install the typescript module so we can lock tsc to a specific version and have access to a project local tsc.

{ test: /\.ts?$/, loader: "ts-loader" },
{ test: /\.js?$/, exclude: [/node_modules/], loader: "ts-loader" },
{
test: /\.js$/,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this also match .ts?

entry: './src/hls',
resolve: {
// Add `.ts` as a resolvable extension.
extensions: [".ts", ".js"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the project still build without extensions including the ts extension? This was needed so the imports tested that file extension.

modules: false,
targets: {
browsers: [
'chrome >= 55',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think hls.js should target older chromium based browsers
For example, Tizen 2.3 TVs (which still have a large market share) ship with a webkit-based browser that corresponds chrome 22
https://developer.samsung.com/tv/develop/specifications/web-engine-specifications

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a good default would be M47 (>= 47), so we for sure support 2017 models.

I'll run a test with this and report back what happens if we drop the target to chrome 22 for the bundle size.

If it's a huge difference it may make sense to make a new target for very old browsers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There isn't a difference in the resulting output for Chrome 22, 47, or the current 55 value.

All the other browsers include the plugins required.

screen shot 2019-02-03 at 1 40 41 pm

Copy link
Collaborator

Choose a reason for hiding this comment

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

As such the bundle size also stays the same with any of the provided targets.

plugins: [
{
visitor: {
CallExpression: function (espath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why aren't you using babel-polyfill for that?

Copy link
Collaborator

@itsjamie itsjamie Feb 3, 2019

Choose a reason for hiding this comment

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

As explained to me, it's like a scalpel approach, we don't want to include a full polyfill that makes the environment completely ES2015 compliant, if we only use the single ES2015 feature. So the small change ends up being net better.

Tooling automation moved this from Changes Requested to Done Feb 7, 2019
@itsjamie itsjamie deleted the chore/upgrade-webpack branch June 25, 2020 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Tooling
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants