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