🕵 fluentui-web-components-v3 No visual regressions between this PR and main

Perf Analysis (@fluentui/react-components)

No significant results to display.

🕵 fluentuiv8 No visual regressions between this PR and main

📊 Bundle size report

✅ No changes found

🕵 FluentUIV0 No visual regressions between this PR and main

Perf Analysis (@fluentui/react-northstar)

Perf Analysis (@fluentui/react)

No significant results to display.

Created issue for leaking types #32194

Hi @Hotell,

Using skipLibCheck to resolve conflicts with the TypeScript standard library is discouraged, as stated in the documentation for skipLibCheck. It's recommended to use lib replacement.

I reverted the moduleResolution changes shortly after creating the PR (I missed a reference which I have now reverted as well, and updated the PR comment to remove that item). It's been a recurring issue that the module and moduleResolution fields show misconfiguration errors in my editor. I'm sorry for any confusion there.

We're not okay with shipping leaking types, which is what this PR is specifically intended to address.

Writing a custom script to do what a few simple, standard, and recommended configuration adjustments already do, in a built-in way, doesn't make sense to me.

I reduced the changes down to only those related to the lib replacement, and fixing the type error in a utility class.

I started looking into the leaking types issue and couldn't quite replicate your red squiggles, but after talking with @radium-v it seems that this PR would help fix the issue with ToggleEvent (a new but valid web platform event object type).

As mentioned in the initial issue...

• /// <reference types="web" /> is causing some issues in downstream consumers.
• Downstream consumers could add @types/web to their tsconfigs, but if they're using newer versions of TS (5.5) they'll run into duplicate type errors.
• We can't tell consumers to use skipLibCheck
• The skipLibCheck documentation seems to push people away from using skipLibCheck to solve this usecase.
• The skipLibCheck documentation is pushing us towards using the lib replacement strategy
• The @types/web documentation is also pushing us towards the lib replacement strategy

Could explain more about the concerns you have for the lib-replacement strategy landing in the web-components package? I'd like to fully understand the problems and perceived complexity.

folks, quick tip: if you wanna another review please make sure to hit the refresh button, otherwise reviewers will never get notification that there has been some movement. ty !

/// is causing some issues in downstream consumers

yes I'm aware what problem are we trying to solve.

my proposal how to mitigate this is postprocessing within build step -> simple script to remove those reference pragmas, instead going route against monorepo setup and practices set in place ( NOTE: it's ok to opt out short term, if there is no clear workaround - which is not this case )

🐞 NOTE: as I mentioned my(your) proposal will not mitigate leaking invalid public API problem.

We can't tell consumers to use skipLibCheck

true, but I'd suggest you to start encourage them to do so. Not using skipLibCheck:true in applications/feature teams adds performance penalty of invoking tsc without gaining most of the time any benefits.

To add, our repo uses skipLibCheck in all packages. only use-case where this might matter to you is when you are authoring and shipping a library which has TS type emit as public api output and you commit to follow semver with it ( not application ) - which is our case ( we disable skipLibCheck when we generate public api interfaces -> .d.ts to make sure we don't ship something that might break another library that builds on ours )

seems to push people away

I guess everyone might perceive it differently, I don't see any "push aways" in those official docs. It would be nice to mention the perf and general recommendations in these docs indeed.

The leaking types issue:

• we remove ref=types pragmas
• some extension of wc library install fluent package
• that team is not using skipLibCheck and they use some older version of typescript that doesn't ship the new DOM api interfaces as part standard lib
• they gonna import in their production code one of the WC api that use those NEW DOM interfaces as part of its public API

// NOTE: super contrived example
import {Menu} from '@fluentui/web-components'

const menu = new Menu()

// 🚨 TS error
menu.toggleHandler(....)

• tsc will fail

hope this answers your questions @davatron5000 . happy to discuss further if needed. ty

I can "fix" the ToggleEvent issue by rewriting the toggleHandler to not use the appropriate type and just use Event. It sort of defeats the entire purpose of TypeScript, but hopefully that unblocks us there.

That said, the web-components project will always be a bit ahead of the curve on web platform feature we use, so the ToggleEvent issue a convenient example of where we're ahead of the current monorepo version of TS in terms of supported web APIs.

Per our hours of investigation into the issue...

• The types: ["web"] method is a mostly undocumented way of extending web types for TS <=4.4. (see docs)
  • The TS <=4.4 docs are hidden in a dropdown and don't mention the types array technique, but instead recommend using lib: ['SomeESTarget'] or removing lib: ["dom"] if lib already exists.
• Since TS 4.5+, TypeScript and @types/web recommends using the lib-replacement method for extending web types.

We did some experiments yesterday and made the exact same demo repro you posted today and verified the tsc compiler couldn't build in the downstream project. Unfortunately, post-processing removing the /// references pragma from all the .d.ts files only half-solved our problem. Downstream consumers were getting duplicate type errors for all types (web-components' types/web + local-repo's lib-dom types). The easiest way for us to resolve that issue was to use lib-replacement. @radium-v and I can work up a demo/video if that would help demonstrate the issue.

With lib-replacement, the leaking types issue only surfaces if a downstream consumer tries to overrride/extend a function that uses newer types. At that point they are using newer types and would need to upgrade their lib-dom types to web-types. For downstream consumers in that situation (TS <= 5.4) we would not recommend skipLibCheck (per the docs) and recommend the lib-replacement strategy (per the docs). So for us, all paths lead us to lib-replacement.

If the web-component package's use of lib-replacement for web types with nohoist guards in effect breaks other systems in the monorepo in some way, please let us know. But we've done our due diligence here and are trying abide in the one-version policy while not impacting other projects but still allowing us to leverage new web technologies.

types or dependency alias?:

whatever those docs say or encourage, its not completely "up to date/accurate" (also note those docs are 3 years old).

In typescript, if you're dealing with global overrides/extensions , you should use types setting. That's the main purpose of this configuration we have set in place for whole monorepo. If you don't specify types explicitly, typescript will include in the Program anything that lives within node_modules including node_modules/@types.

global leaking types:

That menu event is only one case, there are other occurences in your source code.

Related to this, the proper API contract that types enforces here is actually giving library authors ( us ) proper public API surface information in terms of what are we shipping to users.

💡 If your source code had no issues with leaking these global types, that are not supported in every version of typescript, those reference pragmas would not be emitted at all = your .d.ts contract is solid.

🚨 If you use dependency alias (as you propose in this PR) you'll never know about this problem (only if consumers would start creating issues).

solution/recap:

• we (v-build) don't wanna maintain unnecessary nohoist
• you can recommend any kind of typescript configuration practices to your users that you see fit for your packages
• you can use any kind of @types/web as needed
  • this package should live in root (single version policy -> removed from web-components dependencies), unless other packages need something different ( probably this will never happen )
  • to enable web DOM api overrides, explicit usage of types is needed across any monorepo project IFF necessary
• to mitigate shipping leaking types a simple checker should be build that would fail build if global overrides pragmas referencing @types/web are present in public API surface of .d.ts

happy to talk further

Closing this in favor of #32308