Update webpack@4.11.1 + Use TypeScript compiler instead of Babel #1762

Korilakkuma · 2018-06-06T15:46:45Z

This PR will...

Updated webpack@4.11.1 and webpack.config.js. Along with it, installed webpack-cli.

Why is this Pull Request needed?

The build is faster than webpack@3.5.5. Therefore, I think CI finishes faster.

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

Resolves issues:

Related PR

#1631

Checklist

changes have been done against master branch, and PR does not conflict
no commits have been done in dist folder (we will take care of updating it)
new unit / functional tests have been added (whenever applicable)
API or design changes are documented in API.md

tchakabam · 2018-06-07T12:00:02Z

How much faster? :)

tchakabam

I would still need to test it, but looks good so far 👍

tchakabam · 2018-06-07T12:00:25Z

webpack.config.js

@@ -14,19 +14,6 @@ const addAltAudioSupport = !!env.ALT_AUDIO || !!env.USE_ALT_AUDIO;
 const addEMESupport = !!env.EME_DRM || !!env.USE_EME_DRM;
 const runAnalyzer = !!env.ANALYZE;

-const uglifyJsOptions = {


tchakabam · 2018-06-07T12:02:18Z

@Korilakkuma Not sure if you knew, but the upgrade to webpack 4.x is already included in the Typescript toolchain PR: #1716

Wouldn't you want to continue this work maybe rather? It would be even a much more great contribution to the project!

tchakabam · 2018-06-07T12:21:47Z

@Korilakkuma Also, if you like working on the toolchain, feel free to continue the work on that PR as well :) Or merge all together to one branch! #1631

Korilakkuma · 2018-06-07T12:39:21Z

@tchakabam Thank you for review.

merge all together to one branch!

Namely, I should change the merge destination of this PR to improve-webpack-conf branch (#1631) ?

tchakabam · 2018-06-07T20:27:18Z

@Korilakkuma Maybe the simplest is I will just close #1631 because in the end it's not really much. If you see anything useful there feel free to merge in that branch into yours here. But then again, it's just making sure the OccurenceOrder plugin is not used in dev. You can just copy-paste that over :) The other stuff is pretty cosmetic.

What would be more interesting to do would be to continue your work on the TypeScript branch (#1716), it already contains the upgrade to Webpack v4 which you have done here :)

This is something really important actually, and it would be great if you would want to pick it up.

…nto feature/update-webpack

tchakabam · 2018-06-08T14:28:20Z

package.json

@@ -52,7 +52,7 @@
    "test": "npm run test:unit && npm run test:func",
    "test:unit": "karma start karma.conf.js",
    "test:unit:watch": "karma start karma.conf.js --auto-watch --no-single-run",
-    "test:func": "cross-env BABEL_ENV=test mocha --compilers js:babel-register tests/functional/auto/setup.js --timeout 40000"
+    "test:func": "cross-env BABEL_ENV=test mocha --require ts-node/register tests/functional/auto/setup.js --timeout 40000"


👍 you can probably get rid or rename the BABEL_ENV thing.

We were using ts-node as well in another project for powering the toolchain, as we are writing everything down to the Webpack in Typescript. We were just calling scripts like so: ts-node ./node_modules/.bin/webpack. So that's always still a possibility, just for info.

you can probably get rid or rename the BABEL_ENV thing

👍

not sure what it does or if we need a similar thing in typescript though.

tchakabam · 2018-06-08T17:32:59Z

Cool. Might be ready to merge ... :)

azu

LGTM

azu · 2018-06-09T04:50:08Z

package.json

@@ -51,21 +52,16 @@
    "test": "npm run test:unit && npm run test:func",
    "test:unit": "karma start karma.conf.js",
    "test:unit:watch": "karma start karma.conf.js --auto-watch --no-single-run",
-    "test:func": "cross-env BABEL_ENV=test mocha --compilers js:babel-register tests/functional/auto/setup.js --timeout 40000"
+    "test:func": "cross-env mocha --require ts-node/register tests/functional/auto/setup.js --timeout 40000"


Can you removecross-env?
Because, it does not set env.

OK. I try to remove cross-env 👍

I removed cross-env.

Korilakkuma · 2018-06-10T08:08:33Z

@azu @tchakabam

I removed cross-env. And, I added mode to karma.conf.js in order to correspond to webpack@4.11.1. So, please review again.

tchakabam · 2018-06-11T08:46:41Z

webpack.config.js

  }

-  return [enabledConfig, demoConfig];
+  console.log(
+    `Building Hls.js with webpack config:\n\n${JSON.stringify(configs, null, 4)}\n`


Maybe even better: console.log(Building configs: ${configs.map(config => config.name).join(', ')}.\n);

This will just display the names of all the configs being built :)

tchakabam · 2018-06-11T08:48:01Z

tsconfig.json

+{
+  "compilerOptions": {
+    //"inlineSourceMap": true,
+    "sourceMap": true,


are source-maps working correctly?

are they pointing straight into the js file, or into the tsc compiler output? :)

The source maps are created as external file.

Yes, that's good. But are they "working" ? I mean by that, are you able to debug the demo for example, and map a breakpoint into the original source-files? for example :)

admitted, i could check this myself, but just asking ;)

Sorry. It seems that source map is working.

And I can map a breakpoint into the original source-files.

awesome 👍

noUnusedLocals and noUnusedParameters might be good options to add

tchakabam · 2018-06-11T08:48:22Z

Very cool, just a few details to verifiy 👍

…ng built

tchakabam · 2018-06-11T14:08:38Z

@tjenkinson Do you see anything else tbd? :)

tjenkinson

Looks good to me!

tchakabam · 2018-06-12T23:15:25Z

OK. This should be good. But before we merge, I guess we should just validate this on IE11 "just to be sure". If I have well understood what Babel was doing, and what tsc should be doing, this should be fine. However, chances are that we haven't completely gotten it, and that we are still using some thingie that Babel was inserting a polyfill for and IE11 doesn't support natively. Even if our assumption is currently that Babel (in our current configuration) wasn't polyfilling anything, but only transforming ES6 constructs into ES5. So let's better make sure. And then merge + enjoy :)

tchakabam · 2018-06-13T09:30:17Z

IE ✅

tchakabam · 2018-06-13T09:36:40Z

@Korilakkuma Very last thing: the package-lock file should be different, probably as you set karma to v3 again, you should have an unstaged change there as well after an install. Can you re-run npm install just to make sure, and commit it again?

Korilakkuma · 2018-06-13T12:30:16Z

@tchakabam

I tried above procedure. But package-lock.json is not difference to pushed.

tchakabam · 2018-06-13T14:48:40Z

@Korilakkuma Thanks for checking this out 👍 This probably makes sense. Sorry I had you do this first, but I just checked what the diff on the package-lock actually is here for me, and I got this:

diff --git a/package-lock.json b/package-lock.json
index 6ce8a099..bb47af9a 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1194,7 +1194,7 @@
     },
     "buffer": {
       "version": "4.9.1",
-      "resolved": "https://registry.npmjs.org/buffer/-/buffer-4.9.1.tgz",
+      "resolved": "http://registry.npmjs.org/buffer/-/buffer-4.9.1.tgz",
       "integrity": "sha1-bRu2AbB6TvztlwlBMgkwJ8lbwpg=",
       "dev": true,
       "requires": {
@@ -5914,7 +5914,7 @@
     },
     "ip": {
       "version": "1.1.5",
-      "resolved": "https://registry.npmjs.org/ip/-/ip-1.1.5.tgz",
+      "resolved": "http://registry.npmjs.org/ip/-/ip-1.1.5.tgz",
       "integrity": "sha1-vd7XARQpCCjAoDnnLvJfWq7ENUo=",
       "dev": true
     },
@@ -10729,7 +10729,7 @@
     },
     "through2": {
       "version": "1.1.1",
-      "resolved": "https://registry.npmjs.org/through2/-/through2-1.1.1.tgz",
+      "resolved": "http://registry.npmjs.org/through2/-/through2-1.1.1.tgz",
       "integrity": "sha1-CEfLxESfNAVXTb3M2buEG4OsNUU=",
       "dev": true,
       "requires": {
@@ -11257,7 +11257,7 @@
     },
     "tty-browserify": {
       "version": "0.0.0",
-      "resolved": "https://registry.npmjs.org/tty-browserify/-/tty-browserify-0.0.0.tgz",
+      "resolved": "http://registry.npmjs.org/tty-browserify/-/tty-browserify-0.0.0.tgz",
       "integrity": "sha1-oVe6QC2iTpv5V/mqadUk7tQpAaY=",
       "dev": true
     },

No idea why these few assets get resolved through http rather than https on my system :D But it's definitely not an actual issue.

tchakabam · 2018-06-13T14:49:28Z

⚠️ This will get merged to master right after we released 0.10.0, which should probably happen soon 👍

tchakabam · 2018-06-14T23:24:22Z

Before we merge this we could/should:

adapt the readme to mention TS (mentioning Babel currently)
add the esdoc plugin for typescript so that we get API docs generated based on type-declarations
add necessary eslint config for typescript (plugin etc)

tchakabam · 2018-06-19T12:08:33Z

@Korilakkuma we would like to merge this soon, but with the todos above (so that we can seemlessly move on with converting components to TS). therefore, i would merge this here into a typescript branch on our repo, and then quickly add the remaining things, and merge everything to master, finally.

Korilakkuma · 2018-06-19T12:48:39Z

@tchakabam

I changed master to replace-babel-by-typescript. Is this correct ?

tchakabam · 2018-06-20T10:36:54Z

@Korilakkuma Thanks!

tchakabam · 2018-06-25T12:49:22Z

@Korilakkuma @tjenkinson @azu -> #1800

tchakabam and others added 4 commits May 15, 2018 23:28

Webpack: Use Typescript compiler instead of Babel

751e5ba

Update webpack@4.11.1 and Add webpack-cli

380a28f

Correspond to webpack@4.11.1

a1cecf0

Update packages for funcTests

38bff19

tchakabam reviewed Jun 7, 2018

View reviewed changes

tchakabam added the Duplicate label Jun 7, 2018

Merge remote-tracking branch 'upstream/replace-babel-by-typescript' i…

7158e54

…nto feature/update-webpack

tchakabam reviewed Jun 8, 2018

View reviewed changes

tchakabam changed the title ~~Update webpack@4.11.1~~ Update webpack@4.11.1 + Use TypeScript compiler instead of Babel Jun 8, 2018

tchakabam mentioned this pull request Jun 8, 2018

Webpack: Use Typescript compiler instead of Babel #1716

Closed

5 tasks

tchakabam removed the Duplicate label Jun 8, 2018

Use ts-node instead of babel-register

5a81d71

azu approved these changes Jun 9, 2018

View reviewed changes

Korilakkuma added 3 commits June 9, 2018 14:05

Remove cross-env

84aafd2

Add mode

53c2c72

Merge branch 'master' into feature/update-webpack

dea9614

tchakabam reviewed Jun 11, 2018

View reviewed changes

Fixed webpack.config.js to display the names of all the configs bei…

c821376

…ng built

tchakabam self-requested a review June 11, 2018 14:07

tchakabam approved these changes Jun 11, 2018

View reviewed changes

Korilakkuma added 5 commits June 12, 2018 08:53

Removed comment

a94b561

Removed .tsx

f41389d

Added comment for globalObject option

8a9a257

Installed karma-webpack@3.0.0

afdd25f

Enable 'strict' option

4105a42

tjenkinson approved these changes Jun 12, 2018

View reviewed changes

tchakabam modified the milestones: 1.0.0, 0.10.0, 0.10.1 Jun 13, 2018

tchakabam modified the milestones: 0.10.1, 0.10.2 Jun 13, 2018

Korilakkuma changed the base branch from master to replace-babel-by-typescript June 19, 2018 12:42

tchakabam changed the base branch from replace-babel-by-typescript to typescript-finalize June 25, 2018 10:28

tchakabam merged commit 44efa98 into video-dev:typescript-finalize Jun 25, 2018

tchakabam mentioned this pull request Jun 25, 2018

Migration to Typescript compiler #1800

Merged

4 tasks

Korilakkuma deleted the feature/update-webpack branch August 15, 2018 12:20

Update webpack@4.11.1 + Use TypeScript compiler instead of Babel #1762

Update webpack@4.11.1 + Use TypeScript compiler instead of Babel #1762

Conversation

Korilakkuma commented Jun 6, 2018 • edited

This PR will...

Why is this Pull Request needed?

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

Resolves issues:

Related PR

Checklist

tchakabam commented Jun 7, 2018

tchakabam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchakabam commented Jun 7, 2018 • edited

tchakabam commented Jun 7, 2018

Korilakkuma commented Jun 7, 2018

tchakabam commented Jun 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchakabam commented Jun 8, 2018

azu left a comment

Choose a reason for hiding this comment

azu Jun 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Korilakkuma Jun 10, 2018 • edited

Choose a reason for hiding this comment

Korilakkuma commented Jun 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Korilakkuma Jun 11, 2018 • edited

Choose a reason for hiding this comment

Korilakkuma Jun 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchakabam commented Jun 11, 2018

tchakabam commented Jun 11, 2018

tjenkinson left a comment

Choose a reason for hiding this comment

tchakabam commented Jun 12, 2018

tchakabam commented Jun 13, 2018

tchakabam commented Jun 13, 2018

Korilakkuma commented Jun 13, 2018

tchakabam commented Jun 13, 2018 • edited

tchakabam commented Jun 13, 2018

tchakabam commented Jun 14, 2018 • edited

tchakabam commented Jun 19, 2018

Korilakkuma commented Jun 19, 2018

tchakabam commented Jun 20, 2018

tchakabam commented Jun 25, 2018

Korilakkuma commented Jun 6, 2018 •

edited

tchakabam commented Jun 7, 2018 •

edited

tchakabam commented Jun 7, 2018 •

edited

azu Jun 9, 2018 •

edited

Korilakkuma Jun 10, 2018 •

edited

Korilakkuma Jun 11, 2018 •

edited

Korilakkuma Jun 11, 2018 •

edited

tchakabam commented Jun 13, 2018 •

edited

tchakabam commented Jun 14, 2018 •

edited