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

yue4u · 2023-09-22T10:02:37Z

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`

adopters:
- vue, tsup
pros:
- less configuration
- faster? (no tested)
  - vue switched from @microsoft/api-extractor to rollup-plugin-dts from after 3.3
cons:
- generates wrong code in certain conditions https://github.com/egoist/tsup/issues?q=dts
- currently in maintenance-mode

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 · 2023-09-29T05:39:07Z

yarn.lock

-  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"


modified to contain this fix DefinitelyTyped/DefinitelyTyped@b2505fd

yue4u · 2023-09-29T07:18:59Z

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

0b5vr · 2023-10-03T02:24:26Z

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

0b5vr · 2023-10-03T02:34:06Z

packages/three-vrm/src/VRMUtils/index.ts

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

+export type { deepDispose, removeUnnecessaryJoints, removeUnnecessaryVertices, rotateVRM0 };


why is the line needed?

see #1293 (comment)

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

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.

Ah, I think I finally figured this out.

three-vrm is exporting VRMExpressionLoaderPlugin form three-vrm-core which use types-vrm-0.0 inside

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

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

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

do not bundle dts from types-* packages in intermediate packages three-vrm-* and only bundle them inside three-vrm

can achieve single dts and no deps install

some exports from types-* are still private so maybe hard to use

types may conflict to user installed types-*

introduce inconstancy

treat all types-* as dependencies or peerdependencies and let user install them

not true dts bundle but flexible

option to not install types-vrm-0.0 at all

re-export or provide a public interface for each types-* in three-vrm-*

can achieve single dts and no deps install

needs dependency design?

What do you think?

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.

0b5vr · 2023-10-03T02:38:21Z

packages/three-vrm/src/index.ts

@@ -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';


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

0b5vr · 2023-10-03T02:39:51Z

packages/three-vrm/src/index.ts

@@ -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';


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

yue4u · 2023-10-03T02:40:05Z

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 · 2023-10-03T02:57:59Z

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?

0b5vr · 2023-10-03T03:07:14Z

packages/three-vrm-core/src/expressions/index.ts

+import type * as V0VRM from '@pixiv/types-vrm-0.0';
+export type { V0VRM };


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 mentioned this pull request Sep 22, 2023

Tracking issue: Revamp build process #1294

Open

6 tasks

yue4u force-pushed the revamp-build-process branch from 2dc011e to 19ed32d Compare September 27, 2023 10:05

yue4u mentioned this pull request Sep 29, 2023

BREAKING: chore: remove downlevel-dts #1302

Open

yue4u force-pushed the revamp-build-process branch from 19ed32d to d307480 Compare September 29, 2023 04:11

yue4u changed the base branch from dev to chore/remove-downlevel-dts September 29, 2023 04:12

yue4u force-pushed the revamp-build-process branch from d307480 to cb48d0a Compare September 29, 2023 04:14

feat: bundle dts via api-extractor and move deps to dev only

a7bb7b5

yue4u force-pushed the revamp-build-process branch from cb48d0a to a7bb7b5 Compare September 29, 2023 05:38

yue4u commented Sep 29, 2023

View reviewed changes

fix: resolve missing imports

fdb0c87

yue4u requested a review from 0b5vr September 29, 2023 07:20

yue4u marked this pull request as ready for review September 29, 2023 07:20

0b5vr reviewed Oct 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yue4u commented Sep 22, 2023 •

edited

yue4u Sep 29, 2023

yue4u commented Sep 29, 2023

0b5vr commented Oct 3, 2023

0b5vr Oct 3, 2023

yue4u Oct 3, 2023

0b5vr Oct 3, 2023

yue4u Oct 3, 2023

yue4u Oct 3, 2023 •

edited

0b5vr Oct 24, 2023

0b5vr Oct 3, 2023

0b5vr Oct 3, 2023

yue4u commented Oct 3, 2023 •

edited

0b5vr commented Oct 3, 2023

0b5vr Oct 3, 2023

		import type * as V0VRM from '@pixiv/types-vrm-0.0';
		export type { V0VRM };

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

Are you sure you want to change the base?

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

Conversation

yue4u commented Sep 22, 2023 • edited

OPTIONS:

@microsoft/api-extractor

rollup-plugin-dts

do not bundle at all

Needs discussion:

Choose a reason for hiding this comment

yue4u commented Sep 29, 2023

0b5vr commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yue4u Oct 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yue4u commented Oct 3, 2023 • edited

0b5vr commented Oct 3, 2023

Choose a reason for hiding this comment

yue4u commented Sep 22, 2023 •

edited

`@microsoft/api-extractor`

`rollup-plugin-dts`

yue4u Oct 3, 2023 •

edited

yue4u commented Oct 3, 2023 •

edited