Issue 1030: Fix broken Typescript type declarations #3331

RaymondCheng · 2021-04-14T21:32:33Z

Description

Issue 1030: Fix broken Typescript type declarations

Partially fixes #1030. The issue was created to track a problem using
Shaka from a Typescript project. Inside the issue, a fix was contributed
by @alexandercerutti, although it was acknowledged that the fix was not
a complete fix. Event types are still missing, for instance. Having said
that, it seems the main reason that @alexandercerutti's partial fix
wasn't applied is that @joeyparrish expressed a desire for a small
Typescript sample to be added as an automated test to verify that this
works.

Changes

Modified generateTsDefs.py to apply the partial fix by
@alexandercerutti.
While testing, I discovered that the Windows build produces a broken
.d.ts, because the bytes on Windows (b'??.clutz.') are different
than Linux (b'\xe0\xb2\xa0\xe0\xb2\xa0.clutz.'). Fixed by adding a
second replace pattern for Windows. Since the two are mutually
exclusive, I just run them serially without condition.
While verifying, I noticed some unreplaced clutz namespaces. These
occur because instead of ".clutz." the pattern is ".clutz ". I looked
into a couple, they seem to have been supplied because the Closure
compiler lacks declarations. Added Linux and Windows replacement lines
to assign these to the "shakaExterns" namespace.
Added a simple Typescript compilation test. I structured the
tsconfig.json to look like a typical usage case where shaka-player is
imported as a node module. Note that if we set skipLibCheck to false,
this leads to the errors described in Issue Shaka Typescript definition overriding Google Cast Sender with incomplete defs #3185, eg. error TS2694:
Namespace 'google.ima.dai.api' has no exported member 'StreamRequest'
(also mentioned in Issue Generate TypeScript typings #1030). NOTE that when this test fails, I
couldn’t figure out how to get the compiler output on-screen, so instead
I provide instructions for re-running the test interactively, which will
produce the error on-screen.

Testing Procedure

python build/test.py. Confirm pass.
Verify that some of the previously unreplaced clutz namespaces, such
as ಠ_ಠ.clutz.AirPlayEvent, are no longer under the ಠ_ಠ.clutz prefix
and instead are under the shakaExterns namespace, and
that you can compile successfully.
Repeat both of the above steps under both Linux and Windows. Confirm
that the produced .d.ts files differ only in the comments, where the
source file path is stated.

Screenshots (optional)

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to
not work as expected)
This change requires a documentation update

Checklist:

I have signed the Google CLA https://cla.developers.google.com
My code follows the style guidelines of this project
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
I have verified my change on multiple browsers on different platforms
I have run ./build/all.py and the build passes
I have run ./build/test.py and all tests pass

joeyparrish · 2021-04-14T23:50:54Z

This is wonderful and very detailed. Thanks! I'll test it soon in one of Google's internal TypeScript environments to make sure it works there, and if so, I'll merge it.

Are you interested to port a simple example from one of our tutorials to TypeScript so that we can use it in an automated test to verify the generated defs?

@alexandercerutti

Partially fixes shaka-project#1030. The issue was created to track a problem using Shaka from a Typescript project. Inside the issue, a fix was contributed by @alexandercerutti, although it was acknowledged that the fix was not a complete fix. Event types are still missing, for instance. Having said that, it seems the main reason that @alexandercerutti's partial fix wasn't applied is that @joeyparrish expressed a desire for a small Typescript sample to be added as an automated test to verify that this works. Changes 1) Modified generateTsDefs.py to apply the partial fix by @alexandercerutti. 2) While testing, I discovered that the Windows build produces a broken .d.ts, because the bytes on Windows (b'?_?.clutz.') are different than Linux (b'\xe0\xb2\xa0_\xe0\xb2\xa0.clutz.'). Fixed by adding a second replace pattern for Windows. Since the two are mutually exclusive, I just run them serially without condition. 3) While verifying, I noticed some unreplaced clutz namespaces. These occur because instead of ".clutz." the pattern is ".clutz ". I looked into a couple, they seem to have been supplied because the Closure compiler lacks declarations. Added Linux and Windows replacement lines to assign these to the "shakaExterns" namespace. 4) Added a simple Typescript compilation test. I structured the tsconfig.json to look like a typical usage case where shaka-player is imported as a node module. Note that if we set skipLibCheck to false, this leads to the errors described in Issue shaka-project#3185, eg. error TS2694: Namespace 'google.ima.dai.api' has no exported member 'StreamRequest' (also mentioned in Issue shaka-project#1030). NOTE that when this test fails, I couldn’t figure out how to get the compiler output on-screen, so instead I provide instructions for re-running the test interactively, which will produce the error on-screen. Testing Procedure 1) python build/test.py. Confirm pass. 2) Verify that some of the previously unreplaced clutz namespaces, such as ಠ_ಠ.clutz.AirPlayEvent, are no longer under the ಠ_ಠ.clutz prefix and instead are under the shakaExterns namespace, and that you can compile successfully. 3) Repeat both of the above steps under both Linux and Windows. Confirm that the produced .d.ts files differ only in the comments, where the source file path is stated.

RaymondCheng · 2021-04-15T10:06:11Z

Thanks! I went ahead and added a Typescript compilation test. This compilation test can also reproduce Issue #3185 if you change skipLibCheck to false (at least, in part).

joeyparrish

This looks great! Thanks for contributing.

joeyparrish · 2021-04-20T16:39:51Z

tsconfig.json

+    /* Basic Options */
+    "target": "es2015",
+    "module": "es2015",
+    "skipLibCheck": true,


Why skipLibCheck?

I found that if I use default value of skipLibCheck false, then it reproduces Issue #3185 (#3185): error TS2694: Namespace 'google.ima.dai.api' has no exported member 'StreamRequest' (also mentioned in Issue #1030). So, setting skipLibCheck = true is a workaround for now until Issue #3185 is fixed.

Ah, I see. Can you please add a comment to that effect, with a link to #3185? That will help us follow up and fix it later.

joeyparrish · 2021-04-20T16:41:20Z

test/typescript.ts

+
+import * as shaka from 'shaka-player'
+
+const videoSegmentIndex = new shaka.media.SegmentIndex([]);


Do you have time/energy to expand this into something like a full app setup? Something that covers the same basics as our first tutorial? https://shaka-player-demo.appspot.com/docs/api/tutorial-basic-usage.html

I can look into it, but might not be able to start until next week.

joeyparrish · 2021-04-20T16:49:46Z

build/test.py

@@ -413,6 +413,11 @@ def RunCommand(self, karma_conf):
      logging.error('Failed to update node modules')
      return 1

+    if not shakaBuildHelpers.test_typescript():


I don't think this belongs here. The way our build system is organized (it's complex and unique, I know), we would put this into build/check.py, not build/test.py. And shakaBuildHelpers is really for things that need to be shared among test scripts, so you could move the whole function build/check.py. Thanks!

joeyparrish · 2021-04-20T17:06:06Z

package.json

@@ -71,7 +72,8 @@
  },
  "license": "Apache-2.0",
  "scripts": {
-    "prepublishOnly": "python build/checkversion.py && python build/all.py --force"
+    "prepublishOnly": "python build/checkversion.py && python build/all.py --force",
+    "testTypescript": "tsc -p tsconfig.json"


Instead of putting this into package.json and calling "npm run ...", you should call "tsc" directly. We have helpers for this:

base = shakaBuildHelpers.get_source_base() tsc = shakaBuildHelpers.get_node_binary('typescript', 'tsc') command_line = tsc + ['-p', 'tsconfig.json']

The reason for the way we do things is that we have Python-based internal infrastructure tools that import these python scripts as modules. Some day, we may migrate to something more familiar to the node.js world, but until we can also replace those internal tools, it helps to be consistent. Thanks!

avelad · 2022-05-23T07:04:24Z

@RaymondCheng Can you check Joey's comments and rebase?

avelad · 2022-05-30T14:28:33Z

Closing due to inactivity. If you need to reopen this issue, just put @shaka-bot reopen in a comment. Thanks!

joeyparrish self-assigned this Apr 14, 2021

RaymondCheng force-pushed the raych/issue1030_typescriptTypings branch from 4dd8214 to 3b3f45e Compare April 15, 2021 05:15

RaymondCheng force-pushed the raych/issue1030_typescriptTypings branch from 3b3f45e to 6352bed Compare April 15, 2021 10:00

joeyparrish requested changes Apr 20, 2021

View reviewed changes

avelad added type: bug Something isn't working correctly status: waiting on response Waiting on a response from the reporter(s) of the issue labels May 23, 2022

avelad closed this May 30, 2022

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

github-actions bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1030: Fix broken Typescript type declarations #3331

Issue 1030: Fix broken Typescript type declarations #3331

RaymondCheng commented Apr 14, 2021 •

edited

Loading

joeyparrish commented Apr 14, 2021

RaymondCheng commented Apr 15, 2021

joeyparrish left a comment

joeyparrish Apr 20, 2021

RaymondCheng Apr 20, 2021

joeyparrish Apr 20, 2021

joeyparrish Apr 20, 2021

RaymondCheng Apr 20, 2021

joeyparrish Apr 20, 2021

joeyparrish Apr 20, 2021

avelad commented May 23, 2022

avelad commented May 30, 2022


		import * as shaka from 'shaka-player'

		const videoSegmentIndex = new shaka.media.SegmentIndex([]);

Issue 1030: Fix broken Typescript type declarations #3331

Issue 1030: Fix broken Typescript type declarations #3331

Conversation

RaymondCheng commented Apr 14, 2021 • edited Loading

Description

Screenshots (optional)

Type of change

Checklist:

joeyparrish commented Apr 14, 2021

RaymondCheng commented Apr 15, 2021

joeyparrish left a comment

Choose a reason for hiding this comment

joeyparrish Apr 20, 2021

Choose a reason for hiding this comment

RaymondCheng Apr 20, 2021

Choose a reason for hiding this comment

joeyparrish Apr 20, 2021

Choose a reason for hiding this comment

joeyparrish Apr 20, 2021

Choose a reason for hiding this comment

RaymondCheng Apr 20, 2021

Choose a reason for hiding this comment

joeyparrish Apr 20, 2021

Choose a reason for hiding this comment

joeyparrish Apr 20, 2021

Choose a reason for hiding this comment

avelad commented May 23, 2022

avelad commented May 30, 2022

RaymondCheng commented Apr 14, 2021 •

edited

Loading