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

bug: TypeScript target/lib option doesn't constraint ECMA features on type checking within packages #17101

Open
Hotell opened this issue Feb 22, 2021 · 5 comments
Assignees
Labels
Area: Build System Area: Packaging Area: Typescript Fluent UI react (v8) Issues about @fluentui/react (v8) Needs: Discussion Resolution: Soft Close Soft closing inactive issues over a certain period

Comments

@Hotell
Copy link
Contributor

Hotell commented Feb 22, 2021

Library

@fluentui/react - all v8 packages

Current Behaviour

While implementing #16976 I ran into following TS related(compile/runtime) bug within our packages.

  • target/lib tsconfig option restriction has effect on code that is used within monorepo package

image

Expected behavior:

I should get TS errors based on target/lib setting.

Why is this happening?

The issues starts within react-conformance package

image

  • because we are using ES2015 features, those 2 functions will infer return type based on node globals and thus will end up
    with following output
    image
  • node types are explicitly importing ECMA2015+ libs from TS standard lib thus adding those to TS eval scope
    • image

Now because this, any API that is used from this package will "extend" (pollute) TS eval scope of particular package where it's being used (in our example react-menu)

👉 That's why we don't see any errors in our react-menu (event though we are using ECMA2015+ features)

Let's say we'd fix consoleUtil.ts (implementing Object.assign on our own for example), the main problem would not go away.

Why?

  • if we briefly skim through react-conformance implementation, we would discover that there is enzyme imported (within defaultTests.tsx
  • now enzyme types add cheerio types (/// <reference types="cheerio" />) and cherio types add node types (/// <reference types="node" />) so we end up with the same issue

Conclusion
Now even if we remove enzyme and other related culprits, nothing will stop us to introduce such a anomaly in future, because we are mixing 2 environments (with storybook in place 3) within 1 TS config, those leaks like this are inevitable.

An elegant solution for this problem was already introduced in (this RFC), - tsconfig per environment consolidated via TS Solution style config.

Also the solution in more detail: https://gist.github.com/Hotell/19e776aa1a47258340b8bd9b477ad303

@gouttierre
Copy link
Contributor

@Hotell - Thanks for the feedback and suggestion.

@khmakoto - would you be able to follow this one up ? I am not sure if you will need to include talks with @ecraig12345?

@Hotell
Copy link
Contributor Author

Hotell commented Mar 23, 2021

What needs to be done:

  • create RFC with proposed solution
  • merge RFC
  • prioritise and plan actual implementation in particular team with estimation
  • execute implementation in particular milestone on 1 package ( react-menu is great candidate)
  • implement migration script that every package can migrate to this new behaviour gradually
  • set priority and milestone delivery per package with this new fixed behaviour

ling1726 added a commit to ling1726/fluentui that referenced this issue Mar 23, 2021
This PR moves the descendants utility from `react-accordion` to
`react-utilities` and adds inline polyfills because of type leakage in
component packages from `react-conformance` (see microsoft#17101)
ling1726 added a commit that referenced this issue Mar 24, 2021
* Move descendants to react-utilities

This PR moves the descendants utility from `react-accordion` to
`react-utilities` and adds inline polyfills because of type leakage in
component packages from `react-conformance` (see #17101)

* Change files

* undo tsconfig change
joshualamusga1 pushed a commit to joshualamusga1/fluentui that referenced this issue Mar 25, 2021
* Move descendants to react-utilities

This PR moves the descendants utility from `react-accordion` to
`react-utilities` and adds inline polyfills because of type leakage in
component packages from `react-conformance` (see microsoft#17101)

* Change files

* undo tsconfig change
miroslavstastny pushed a commit to miroslavstastny/fluentui that referenced this issue May 5, 2021
* Move descendants to react-utilities

This PR moves the descendants utility from `react-accordion` to
`react-utilities` and adds inline polyfills because of type leakage in
component packages from `react-conformance` (see microsoft#17101)

* Change files

* undo tsconfig change
@khmakoto khmakoto assigned Hotell and unassigned joschect May 17, 2021
@msft-fluent-ui-bot
Copy link
Collaborator

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

@msft-fluent-ui-bot msft-fluent-ui-bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Dec 12, 2021
@ecraig12345 ecraig12345 reopened this Dec 13, 2021
@ecraig12345 ecraig12345 removed the Resolution: Soft Close Soft closing inactive issues over a certain period label Dec 13, 2021
@msft-fluent-ui-bot
Copy link
Collaborator

Because this issue has not had activity for over 180 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

@msft-fluent-ui-bot
Copy link
Collaborator

Because this issue has not had activity for over 180 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

@microsoft microsoft locked as resolved and limited conversation to collaborators Jan 10, 2023
@Hotell Hotell reopened this Jul 20, 2023
@Hotell Hotell added Fluent UI react (v8) Issues about @fluentui/react (v8) and removed Resolution: Soft Close Soft closing inactive issues over a certain period labels Jul 20, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Jan 16, 2024
@Hotell Hotell removed the Resolution: Soft Close Soft closing inactive issues over a certain period label Jan 17, 2024
@Hotell Hotell reopened this Jan 17, 2024
@Hotell Hotell changed the title TypeScript target/lib option doesn't constraint ECMA features on type checking within packages fluentui/react: TypeScript target/lib option doesn't constraint ECMA features on type checking within packages Jan 17, 2024
@Hotell Hotell changed the title fluentui/react: TypeScript target/lib option doesn't constraint ECMA features on type checking within packages bug: TypeScript target/lib option doesn't constraint ECMA features on type checking within packages Mar 26, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Sep 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: Build System Area: Packaging Area: Typescript Fluent UI react (v8) Issues about @fluentui/react (v8) Needs: Discussion Resolution: Soft Close Soft closing inactive issues over a certain period
Projects
None yet
Development

No branches or pull requests

5 participants