Consider migration to goog.module

[joeyparrish]

goog.module is now preferred by the compiler over goog.provide. According to internal advice I've just received, goog.module will allow the compiler to check dependencies better, among other things.

We should consider migrating, especially if it does not introduce any compatibility issues. This will probably involve changes to our extern generator.

https://github.com/google/closure-library/wiki/goog.module:-an-ES6-module-like-alternative-to-goog.provide

[avelad]

We still use AngularJS, please note the workaround to support AngularJS. https://github.com/google/closure-library/wiki/goog.module:-an-ES6-module-like-alternative-to-goog.provide#googmodule-causes-problems-with-angular-injected-classes

goog.module causes problems with Angular injected classes
By default a goog.module file's exports are sealed. If the export object is a constructor, then AngularJS won’t be able to add the $inject property it expects to. This happens both during development (AngularJS adds the $inject property to cache injections) and during production when using --angular_pass.

You can work around this by disabling using the goog.SEAL_MODULE_EXPORTS define.

[joeyparrish]

Excellent. Thanks for pointing that out!

[joeyparrish]

@IsaacSNK suggested in #4423 (comment):

Is it worth migrating start by changing the code to use ESModules instead of goog.require and goog.provide? Assumming that Closure plays well with ESModules. I'm thinking that doing small incremental changes like that will make it easier to fully migrate to something else

and in #4423 (comment):

I can definitely contribute with a few hours of my time every week.

Also, in #4016, @dulmandakh started the migration process, but ran into issues.

In #4423 (comment), I suggested:

If people would like to experiment with this, I would suggest we start a new branch for it and PR into that branch. I would want to avoid doing releases from main with a half-complete implementation of a goog.module transition.

Furthermore, there are unique scripts that might need adjustment or replacement as part of the transition, namely build/generateExterns.js and build/generateTsDefs.py. If this complicates the process too much, they could be temporarily disabled, but this would need to be dealt with before a merge into main.

Okay, I think this consolidates everything into one place.

[IsaacSNK]

Hi! Next week, I plan to work full time on this. I'll be posting ideas and questions. I'd really appreciate any feedback or advice you can provide me. Thanks!

[IsaacSNK]

Sharing some ideas I have about this:

1. Migration should be done through a tool. Manually converting files will be very slow and synching changes done in parallel to the project will be troublesome.
2. Option A: Migrate to ES6 Modules. As I read in several places, this can be done in two steps, first convert to goog.module and then to ES6 modules (this could be done in two separate PRs). I found this script (https://github.com/google/blockly/blob/master/scripts/goog_module/convert-file.sh) that does part of the conversion. I tried it already but there are many things to fix that the tools is not considered, but I can definitely use it as a starting point.
3. Option B: Migrate to Typescript: Given that TS is the ultimate goal, I feel like this is the right way. To reduce the amount of changes, we can start by using Typescript namespace instead of modules. Migrating to modules is more intrusive but can be done incrementally later.

Thoughts?

Other questions/comments related to option B:

1. Externs are no longer needed.
2. Build scripts are no longer needed.
3. How can we generate documentation from the typescript code?
4. Clients can consume the NPM package unbundled and then use their own bundle, minifier and treeshaker. But for clients relying on the bundled file, we also need to include a bundled version in the package.
5. Is there any library that we can use for assersions instead of goog.asserts?
6. For logging, Shaka includes a debug bundle with more logs. How can we do something similar in the new setup?

[IsaacSNK]

@joeyparrish Gents did fairly well converting the code to Typescript. There are several things to fix from the code that it generated. @nana22 is helping me to see how much we can do this week. We are in a company-wide hackathon...

A few things that need conversion (manual or automated) after running gents:

1. Fix imports paths
2. Convert externs to its corresponding type
3. Find a replacement for goog.asserts
4. Remove some leftover goog.require
5. Fix some TS compilation errors

The generated code can be seen here: https://github.com/IsaacSNK/shaka-player alongside each JS file, there it's corresponding .ts file

To automate some tasks, we are using the Typescript Compiler API from a tool we wrote.

[joeyparrish]

Find a replacement for goog.asserts

goog.asserts.assert is just like console.assert, except that the compiler uses it to enhance type info, and it's stripped in a production build. For example, if you assert that a nullable variable is not null at a given point, the compiler will let you pass it where a non-nullable is required.

You can just port the one in lib/debug/asserts.js, which does nothing in production and calls console.assert in a debug build.

[IsaacSNK]

Hey @joeyparrish we were wondering what is google.ima.AdsManager. We see it in Shaka's code but it is not declared in package.json or anywhere else. Could you provide some guidance for this one?

[joeyparrish]

It's an "extern", which is an external interface to another library. In this case, it's the Google IMA SDK.

[IsaacSNK]

And how is that resolved during build time?

[IsaacSNK]

Nevermind, I just found it

[IsaacSNK]

Quick update on this: We were able to convert most of Shaka code to TS. We modified the Gents tool to adjust better to our goals. For many of the compilation errors, we created a tool to add //@ts-ignores. I think this is the best way to avoid lots of code changes, and once the conversion to TS is merged, we can remove //@ts-ignores with more care.

The process we followed to migrate was:

1. Run Gents tool with our changes
2. Manually fix some edge cases not handled by Gents
3. Run Prettier on all the code
4. Run tsc build and generate a report. This report is consumed by a tool we write to add ts-ignore to some of the errors like TS2532, TS2314 and TS2322.

We will be doing manual checks of other compilation errors and see if we should add ts-ignore for now. Work is far from over, but we plan to keep investing a few hours every week. I'll be posting more questions/updates as we move forward.

To keep track of parallel changes happening on the Shaka repo, we have a tool that calculate a hash of each .js and generates a file. Whenever we sync with Shaka latest changes, it will be easy to tell which files changed and need synching.

[avelad]

I do not agree to migrate to TypeScript. Shaka Player currently works on devices with very low features (for example some SmartTVs), if it is migrated to TypeScript there will be a considerable loss of performance, and we would have the same problems that we had when moving from 2.5 to 3.X and which still causes problems at #4062

[IsaacSNK]

@avelad there are many things we can tune during transpilation. Also, once we bundle and minify the code, other optimizations could be done. Your concerns are definitely valid, but I think once we have this work done, we can run performance tests and then decide. Honestly, for me and @nana22 even if this migration never sees the light due to concerns like yours, it will be valuable anyway due to the learning experience and familiarity we are gaining with Shaka.

[littlespex]

@joeyparrish Closure is being sunset in 5 months. Is this ticket still relevant?

https://github.com/google/closure-library/blob/master/README.md
google/closure-library#1214

@avelad What performance losses are you referring to in regards to a TypeScript conversion?

[avelad]

@littlespex The only problem was on WebOS 3.X, but my tests were 3 years ago, the results may have changed. These devices are 7 years old or older, I think it's reasonable to evolve, so I agree to use TypeScript.

[avelad]

Since our plan is to migrate to TypeScript, @joeyparrish, are you okay if we close this issue?

[joeyparrish]

goog.module may be an intermediate stage of the solution, but I don't know that we need a separate tracking bug for it.