[Bug]: All packages have a direct dependency on `@types/*` regardless of whether TypeScript is used

tido64 commented 2 months ago

Library

React / v8 (@fluentui/react)

System Info

n/a

Are you reporting an Accessibility issue?

None

Reproduction

https://github.com/microsoft/rnx-kit/blob/main/.yarnrc.yml#L21

Bug Description

As an example, @fluentui/utilities has a peer dependency on @type/react. This causes package managers to either warn or even fail (default with latest npm) if the consumer of the package does not also install the @types packages. However, it is not always the case that TypeScript is being used. Forcing users to install them in this scenario is unnecessary. Instead, we should mark them all as optional (example).

Logs

No response

Requested priority

Low

Products/sites affected

Anyone not using TypeScript

Are you willing to submit a PR to fix?

yes

Validations

[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] The provided reproduction is a minimal reproducible example of the bug.

tido64 commented 2 months ago

cc @layershifter

Hotell commented 2 months ago

we discussed this today with v-build folks, with following outcome:

immediate action (to unblock your use-case):

@tido64 ~please go ahead with the PR https://github.com/microsoft/fluentui/pull/30964 - but remove the peer dep meta rather than introducing a meta flag which might not be supported by all package managers ?~ ( update )

follow up action:

clean-up dependency definitions in all packages

long term action:

in future we will be generating package.json contents, including dependencies, dynamically based on source file usage + implicit dependencies = this way this kind of thing should never happen as source of truth is source code shipped to registry.

tido64 commented 2 months ago

@tido64 please go ahead with the PR #30964 - but remove the peer dep meta rather than introducing a meta flag which might not be supported by all package managers ?

@Hotell: Just so I understand you correctly, do you want me to remove @types/react from this one package and also remove the peerDependenciesMeta field?

which might not be supported by all package managers

This field was added in npm 7 and seems to be widely adopted. Here's pnpm and Classic Yarn.

Hotell commented 2 months ago

Just so I understand you correctly, do you want me to remove @types/react from this one package and also remove the peerDependenciesMeta field?

yes but hold on ↓

the rule is/should be: if react types are not exposed as part of public API I don't see a reason why it should be there as peerDependency.

how does it look like for utilitities package:

I checked public .d.ts API, and can confirm that react types are part of public API surface

-> so the peerDep should stay as is.

If you are aware about other packages that have the issue and dont use those types as part of public API, happy to accept changes for these.

thoughts ? ty

Hotell commented 2 months ago

This field was added in npm 7 and seems to be widely adopted. Here's pnpm and Classic Yarn.

thanks for quick research ! I can confirm it's supported also for yarn v4

tido64 commented 2 months ago

the rule is/should be: if react types are not exposed as part of public API I don't see a reason why it should be there as peerDependency.

This is why we should introduce peerDependenciesMeta and make it optional. If users don't use TypeScript, they should not have to install @types/* packages. Even if there are some package managers that don't support this field, it would simply be ignored. I don't think we've got anything to lose here.

I checked public .d.ts API, and can confirm that react types are part of public API surface

Why are we re-exporting react? That sounds horrible. There is no way we can guarantee any sort of API stability.

Hotell commented 2 months ago

This is why we should introduce peerDependenciesMeta and make it optional. If users don't use TypeScript, they should not have to install @types/* packages. Even if there are some package managers that don't support this field, it would simply be ignored. I don't think we've got anything to lose here.

That's fair, on the other hand we approach .d.ts that we ship as part of our public API interface (following semver unlike typescript) to provide solid guarantees to our users. Also we are not aware about our library usage without TypeScript.

I also agree that it is not good practice to force download things you don't use.

On the other hand the whole auto peer dependency install is not the most explicit approach that was set as default rather opt in ( but we are very small fish to be able to steer the direction within package managers ).

Side note: if React would ship official type definitions we woulnd't be having this conversation at all, but well that's the state of things

Regarding pros/cons:

Pros:

no changes needed to our stable packages

Cons:

users that don't use TypeScript will get behind the scenes installs of @types/react which footprint (29kB - measured via npm pack) is small and doesn't effect anything runtime specific.

Summary:

we need to double-check our real dependency needs in all packages in future
we won't proceed with the meta field for now, unless more users start complaining
- if necessary we can discuss this on tech sync ( thoughts ? @layershifter )

Happy to hear more thoughts if needed @tido64 🙌

Why are we re-exporting react? That sounds horrible. There is no way we can guarantee any sort of API stability.

That "works as expected" -> its how .d.ts emit works.

utilities package exports as part of its public API React components ( those are annotated via React.Component and friends ) thus the public API (.d.ts needs to import from @types/react )

microsoft-github-policy-service[bot] commented 1 month ago

Because this issue is marked as by design and has not had activity for over 3 days, we're automatically closing it for house-keeping purposes.

microsoft / fluentui