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

build: bundle dts via api-extractor and move deps to dev only #1293

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

yue4u
Copy link
Contributor

@yue4u yue4u commented Sep 22, 2023

close #1289

OPTIONS:

@microsoft/api-extractor

  • adopters:
    • vite, angular, puppeteer
  • pros:
    • actively developed
    • can be used as interface checker
  • cons:
    • needs configuration
    • heavy / maybe over complicated for us

rollup-plugin-dts

do not bundle at all

  • pros:
    • less configuration
    • fast?
    • tree shaking support?
      • if we go esm only it's actually not bad to not bundle at all
  • cons:
    • esm/cjs support
    • cdn may break/hard to try out
    • large install size

Needs discussion:

  • do we need api.md?

@yue4u yue4u changed the base branch from dev to chore/remove-downlevel-dts September 29, 2023 04:12
resolved "https://registry.yarnpkg.com/@types/webxr/-/webxr-0.5.0.tgz#aae1cef3210d88fd4204f8c33385a0bbc4da07c9"
integrity sha512-IUMDPSXnYIbEO2IereEFcgcqfDREOgmbGqtrMpVPpACTU6pltYLwHgVkrnYv0XhWEcjio9sYEfIEzgn3c7nDqA==
version "0.5.5"
resolved "https://registry.yarnpkg.com/@types/webxr/-/webxr-0.5.5.tgz#9e0a27e809c8f76cc1ef525d9f96b8fd94ef9c42"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified to contain this fix DefinitelyTyped/DefinitelyTyped@b2505fd

@yue4u
Copy link
Contributor Author

yue4u commented Sep 29, 2023

fdb0c87 addressed missing import issues. The added exports are most for building the dts bundle.

@yue4u yue4u requested a review from 0b5vr September 29, 2023 07:20
@yue4u yue4u marked this pull request as ready for review September 29, 2023 07:20
@0b5vr
Copy link
Contributor

0b5vr commented Oct 3, 2023

the report files end with .api.md, are those files meant to be committed to?

@@ -3,6 +3,8 @@ import { removeUnnecessaryJoints } from './removeUnnecessaryJoints';
import { removeUnnecessaryVertices } from './removeUnnecessaryVertices';
import { rotateVRM0 } from './rotateVRM0';

export type { deepDispose, removeUnnecessaryJoints, removeUnnecessaryVertices, rotateVRM0 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the line needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mhm,,, How does it work? If I understand that the api-extractor simply forgets to output types for such static members, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added these to avoid api-extractor generating dts with some missing types. But after trying again now the issue has not been fixed. 🤔 Let me check it again.

Copy link
Contributor Author

@yue4u yue4u Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I finally figured this out.

  1. three-vrm is exporting VRMExpressionLoaderPlugin form three-vrm-core which use types-vrm-0.0 inside
  2. if we bundle three-vrm-core with types-vrm-0.0 first, some exports from types-vrm-0.0 became private in three-vrm-core but it works fine
  3. if we bundle three-vrm with bundled three-vrm-core then, the private part is untouchable from three-vrm so the output dts is incomplete
    1. which seems reasonable so it's hard to say it's a bug in api-extractor

The same thing happends to ConstraintSchema.

So our options are

  1. do not bundle dts from types-* packages in intermediate packages three-vrm-* and only bundle them inside three-vrm
    1. can achieve single dts and no deps install
    2. some exports from types-* are still private so maybe hard to use
    3. types may conflict to user installed types-*
    4. introduce inconstancy
  2. treat all types-* as dependencies or peerdependencies and let user install them
    1. not true dts bundle but flexible
    2. option to not install types-vrm-0.0 at all
  3. re-export or provide a public interface for each types-* in three-vrm-*
    1. can achieve single dts and no deps install
    2. needs dependency design?

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like 2 is the best option...? It sounds easier from a contributor perspective at least.
3 is probably the most favorable option from a user perspective but it would make the module hard to maintain.

@@ -9,3 +9,6 @@ export * from '@pixiv/three-vrm-core';
export * from '@pixiv/three-vrm-materials-mtoon';
export * from '@pixiv/three-vrm-node-constraint';
export * from '@pixiv/three-vrm-springbone';

export type { VRMMaterialsV0CompatPlugin } from '@pixiv/three-vrm-materials-v0compat';
export type { VRMNodeConstraintLoaderPlugin } from '@pixiv/three-vrm-node-constraint';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it overwrapped with the line export * from '@pixiv/three-vrm-node-constraint'; ?

@@ -9,3 +9,6 @@ export * from '@pixiv/three-vrm-core';
export * from '@pixiv/three-vrm-materials-mtoon';
export * from '@pixiv/three-vrm-node-constraint';
export * from '@pixiv/three-vrm-springbone';

export type { VRMMaterialsV0CompatPlugin } from '@pixiv/three-vrm-materials-v0compat';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simply expose the entirety of @pixiv/three-vrm-node-constraint since it's already included in the build regardless.

@yue4u
Copy link
Contributor Author

yue4u commented Oct 3, 2023

the report files end with .api.md, are those files meant to be committed to?

As post as Needs discussion: in the description, with committed api.md CI will fail during the build if any unintended public api interface change detected.

cons: may be noisy / pollute search results

@0b5vr
Copy link
Contributor

0b5vr commented Oct 3, 2023

As post as Needs discussion: in the description, with committed api.md CI will fail during the build if any unintended public api interface change detected.

ooo that actually sounds sweet. While I slightly feel that I want to try this, by looking at other adopters no one is using this feature. Would you want to recommend us using the feature?

Comment on lines +12 to +13
import type * as V0VRM from '@pixiv/types-vrm-0.0';
export type { V0VRM };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, these two lines are needed to control whether we are going to expose V0VRM from three-vrm-core or not, right?

Why we need to have this in src/expressions/index.ts , not src/index.ts ?

@yue4u yue4u force-pushed the chore/remove-downlevel-dts branch 9 times, most recently from abe7da1 to 136ce08 Compare June 6, 2024 06:54
@yue4u yue4u force-pushed the chore/remove-downlevel-dts branch from 136ce08 to f0c66b6 Compare June 6, 2024 07:07
Base automatically changed from chore/remove-downlevel-dts to dev-v3 June 10, 2024 10:09
Base automatically changed from dev-v3 to dev August 1, 2024 08:10
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 this pull request may close these issues.

Suggestion: Bundle three-vrm types, move subpackages to devDependencies
2 participants