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

Suggestion: Bundle three-vrm types, move subpackages to devDependencies #1289

Open
mattrossman opened this issue Sep 11, 2023 · 2 comments · May be fixed by #1293
Open

Suggestion: Bundle three-vrm types, move subpackages to devDependencies #1289

mattrossman opened this issue Sep 11, 2023 · 2 comments · May be fixed by #1293

Comments

@mattrossman
Copy link
Contributor

mattrossman commented Sep 11, 2023

I faced some difficulties when trying to make a fix in three-vrm.

The published contents of @pixiv/three-vrm contained .js files that bundle all subpackages together, but the .d.ts still reference individual subpackages from @pixiv/three-vrm-*. Additionally, these subpackages are listed as "dependencies" of three-vrm even though their code is already bundled. These dependencies are only used to provide types for the bundle.

This causes the following issues:

  • Users download unused .js bundles for each subpackage to node_modules
  • Type definitions for three-vrm can get desynchronized from the actual .js code

For example, I tried to fork the repo to make a small change. After building and publishing three-vrm to my own namespace, I experienced strange build errors. Even though my published .js files contained the desired changes, the types were still pointing at the unchanged upstream three-vrm-core package. This means I'd have to publish each subpackage individually to my namespace!

A solution I adopted in my fork is to bundle the types for three-vrm using tsup. This means the published .js and .d.ts files are sure to be in sync, and subpackages are no longer required as "dependencies".

Here is the change: mattrossman@3c1e258

This might make it easier for folks in the community to make fixes, while also reducing the package's footprint in node_modules :)
This shouldn't affect users who wish to install the subpackages individually.

node_modules before node_modules after
CleanShot 2023-09-11 at 14 55 34@2x CleanShot 2023-09-11 at 14 53 58@2x

Just a suggestion—I can make a PR for this if you're interested, otherwise feel free to close.

@yue4u
Copy link
Contributor

yue4u commented Sep 13, 2023

@mattrossman Hi, thanks for the suggestion. The example makes sense to me and the diff looks good overall. Since it changes the build workflow please let us consider it for a while. Much appreciated.

@Benjythebee
Copy link

Yeah for some reason TS is really struggling with the types of three-vrm, it won't find {VRMHumanBoneName} for example. But it will find "{VRM}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants