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

refactor: migrate to goog.module #4016

Closed
wants to merge 2 commits into from

Conversation

dulmandakh
Copy link
Contributor

@dulmandakh dulmandakh commented Mar 8, 2022

I was thinking about migrating Shaka player to JS only tooling, and re-ignited when @joeyparrish said that he is open to TypeScript idea. Then I found that googl.module might ease the migration. Below is an excerpt from goog.module documentation.

Presently goog.module is recommended over goog.provide for new Closure files

Short plan:

  1. Migrate everything to goog.module, more like ES6 module export/import. See https://github.com/google/closure-library/wiki/goog.module:-an-ES6-module-like-alternative-to-goog.provide.
  2. remove goog.module.declareLegacyNamespace(), and make sure everything works as expected.

Then it's easier to migrate to ES6 (using TypeScript in JSDoc) with https://github.com/google/closure-compiler/wiki/Migrating-from-goog.modules-to-ES6-modules.

@joeyparrish could you please review the code and plan

@dulmandakh dulmandakh changed the title migrate to goog.module refactor: migrate to goog.module Mar 8, 2022
@joeyparrish
Copy link
Member

Thank you for starting this process! I think this is a very reasonable plan, and it should make future migration to TS easier.

Are you willing to continue the migration and convert the whole codebase to goog.module? If it's relatively straightforward to do, we could have that included in the upcoming v4 release.

@dulmandakh
Copy link
Contributor Author

dulmandakh commented Mar 8, 2022

@joeyparrish thanks for the comments. I'm not sure if I have time and experience to finish the migration by myself.

Is it possible to merge these changes as these are not breaking?

Also I'm getting below error when I try to convert Dom, BufferUtils, IReleasable, Platform. It seems that we need to change some scripts to fix this.

/shaka-player/dist/shaka-player.ui.debug.externs.js:2259: ERROR - [JSC_UNDEFINED_VARIABLE] variable exports is undeclared
  2257| };
  2258| /**  */
  2259| exports = class Dom {
  2260|   /**
  2261|    * Remove all of the child nodes of an element.

1 error(s), 0 warning(s)
Traceback (most recent call last):
  File "/shaka-player/build/generateTsDefs.py", line 137, in <module>
    main(sys.argv[1:])
  File "/shaka-player/build/generateTsDefs.py", line 132, in main
    GenerateTsDefs(args.input, args.output)
  File "/shaka-player/build/generateTsDefs.py", line 49, in GenerateTsDefs
    contents = shakaBuildHelpers.execute_get_output(command)
  File "/shaka-player/build/shakaBuildHelpers.py", line 187, in execute_get_output
    raise subprocess.CalledProcessError(obj.returncode, args[0], stdout)
subprocess.CalledProcessError: Command 'node' returned non-zero exit status 2.
[ERROR] TS defs generation failed

@joeyparrish
Copy link
Member

Sorry for not responding sooner. We're getting caught up on PRs.

I don't know what to tell you about this, and we don't have time to work on it ourselves right now. If you are stuck, it may have to wait until we're ready to do a full migration.

@joeyparrish
Copy link
Member

Closing this PR for now. We aren't ready to migrate to TS, and I think goog.module should be part of a comprehensive plan and not done piecemeal. But we appreciate your efforts, and thank you for your contribution!

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants