-
-
Notifications
You must be signed in to change notification settings - Fork 13
wasm: apply TypeScript bindings and d.ts to lottie-player.ts #118
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you please check comments
8b721fb
to
69aab05
Compare
I run this patch and found that there is some error. test environmts:
error log:
comment: Is there no such error log in Mac environment? |
@kang-hyuck Thanks for pointing that out! I’ll test it on Ubuntu and update the script to avoid relying on brace expansion. |
69aab05
to
cef0962
Compare
@kang-hyuck I tested it again on my WSL2 and confirmed the same issue. Thanks to your insight, I was able to resolve it. Much appreciated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need tsd generation script here
cef0962
to
4746f88
Compare
@tinyjin Based on the shell script you provided, I modified this PR to create an emit_tsd folder internally and copy only the .d.ts file to the dist folder during the build process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Please check comments
4746f88
to
94fc49d
Compare
d1476c7
to
00fa236
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the TypeScript bindings for lottie-player.ts
to use proper type definitions from the WASM module, addressing the need for better TypeScript support in the WASM output.
Key changes:
- Adds proper TypeScript type definitions by importing
MainModule
andTvgLottieAnimation
types - Replaces generic
any
types with specific type annotations throughout the codebase - Adds null safety checks for module initialization and operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
wasm_emit_tsd.sh | Adds script to generate TypeScript definition files during WASM build process |
src/lottie-player.ts | Updates type annotations and adds null safety checks for WASM module usage |
package.json | Modifies build script to copy generated TypeScript definition files |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
00fa236
to
70888fa
Compare
@tinyjin |
70888fa
to
147d31b
Compare
147d31b
to
267ecc5
Compare
267ecc5
to
3c0885f
Compare
lottie: clean up code That PR should be merged first in order to properly keep the current code structure here. |
3c0885f
to
44e3192
Compare
@tinyjin Could you re-review the PR at your convenience? Thank you. |
44e3192
to
0fac99e
Compare
0fac99e
to
ec2a354
Compare
ec2a354
to
01a3ded
Compare
@kunyoungparkk is attempting to deploy a commit to the thorvg-web Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contribution. Please check comments as well as above suggestion
|
||
private _loadBytes(data: Uint8Array): void { | ||
const isLoaded = this.TVG.load(data, this.fileType, this.canvas!.width, this.canvas!.height, ''); | ||
const isLoaded = this.TVG!.load(data, this.fileType, this.canvas!.width, this.canvas!.height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unrelated changes to TypeScript binding support. Removing unused parameter could be separated in another patch.
this.pause(); | ||
this.currentFrame = curFrame; | ||
this.TVG.frame(curFrame); | ||
this.TVG!.frame(curFrame); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overuse of the !
operator may cause unexpected runtime errors.
Please add an if (!this.TVG)
check at the beginning of the function and remove the non-null assertion.
alias({ | ||
entries: [ | ||
{ find: 'thorvg', replacement: path.join('..', presetMap[preset].path, 'thorvg') }, | ||
{ find: '../dist/thorvg.js', replacement: path.join('..', presetMap[preset].path, 'thorvg') }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong alias...
There is no case ../dist/thorvg.js
in import statement.
// @ts-ignore: WASM Glue code doesn't have type & Only available on build progress | ||
import Module from 'thorvg'; | ||
import Module from '../dist/thorvg'; | ||
import type { MainModule, TvgLottieAnimation } from '../dist/thorvg'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import Module, { type MainModule, type TvgLottieAnimation } from '../dist/thorvg';
|
||
type LottieJson = Map<PropertyKey, any>; | ||
type TvgModule = any; | ||
type TvgModule = TvgLottieAnimation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have type support, we don't actually need this type (No need wrapper type). Instead, let's replace TvgModule
with TvgLottieAnimation
.
const buffer = this.TVG.render(); | ||
const clampedBuffer = new Uint8ClampedArray(buffer.buffer, buffer.byteOffset, buffer.byteLength); | ||
const buffer = this.TVG!.render(); | ||
const clampedBuffer = new Uint8ClampedArray(buffer.buffer as ArrayBuffer, buffer.byteOffset, buffer.byteLength); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as
is a signal that the data type of a value has been wrongly chosen. There is no point in introducing type safety if we bypass it.
see: thorvg/thorvg#3887
check:
const buffer = this.TVG!.render();
const clampedBuffer = new Uint8ClampedArray(buffer, 0, buffer.byteLength);
This PR updates
lottie-player.ts
to adopt the new TypeScript bindings introduced inthorvg#3678
.These updates are related to
thorvg.web#16
, which requests better TypeScript support for the WASM output.