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: exports map point to invalid declarations types which may cause issues for ESM native users #25034

Closed
3 tasks done
Hotell opened this issue Sep 30, 2022 · 2 comments · Fixed by #25558
Closed
3 tasks done

Comments

@Hotell
Copy link
Contributor

Hotell commented Sep 30, 2022

Library

React Components / v9 (@fluentui/react-components)

Describe the issue

We introduced exports maps to our packages some time ago to improve bundling and tree-shakeability.

While doing that we forgot to update types fields within export maps, which currently point to non existing declaration files. This might be potentially dangerous to users that wanna use native ESM and use Typescript >=4.7 with "moduleResolution": "Node16", or "moduleResolution": "NodeNext" configuration.

Have you discussed this feature with our team

v-build

Additional context

Exports map with types field paths are consumed by TS if and only if:

  • compilerOptions.moduleResolution is set to Node16 or NodeNext
    • note that if package has "type":"module" in its package.json typescript doesn't care (but it should based on docs - TS bug ? )

What does this mean to us:

  • our source of truth is typings or types root property in package.json that points to our rolluped file (dist/index.d.ts or dist/unstable.d.ts for unstable)
  • we need to fix all "exports[glob]types" in package json to point to ./dist/index.d.ts and ./dist/unstable.d.ts respectively.
    • Why do we need to fix it ? typescript is "SMART" about type declaration resolution and if user is in native ESM mode, and types are not provided in exports map, TS will "try" to check for module name with alias of js module ( so foo/hello.js will try to resolve foo/hello.d.ts which will throw type error.

🚨 Word of caution:

if consumers are using native ESM modules they are out of luck because v9 doesn't support native ESM modules -> Thus they should not use `compilerOptions.moduleResolution is set to Node16 or NodeNextat all !

Validations

  • Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

Tasks

@Hotell Hotell self-assigned this Sep 30, 2022
@Hotell Hotell changed the title Fix: exports map point to invalid declarations types which may cause issues for ESM native users Bug: exports map point to invalid declarations types which may cause issues for ESM native users Sep 30, 2022
@EvanCahill
Copy link
Member

Even with "moduleResolution": "node" consumers may still face issues with other tools like webpack which restrict imports to paths listed in the exports map. https://webpack.js.org/guides/package-exports/

So while typescript will allow an import like @fluentui/react-theme/lib/utils/createLightTheme webpack will fail with the following error:

[webpack-cli] ModuleNotFoundError: Module not found: Error: Package path ./lib/utils/createLightTheme is not exported from package node_modules@fluentui\react-theme (see exports field in node_modules@fluentui\react-theme\package.json)

@Hotell
Copy link
Contributor Author

Hotell commented Nov 7, 2022

@EvanCahill

So while typescript will allow an import like @fluentui/react-theme/lib/utils/createLightTheme webpack will fail with the following error:

This wont happen with react-components (v9) as we don't allow nested (private) paths since day 1. We generate rolluped dts files which are 1:1 with exports maps thus mitigating issues you pointed out.

@msft-fluent-ui-bot msft-fluent-ui-bot added Status: Fixed Fixed in some PR and removed Status: In PR labels Nov 10, 2022
@microsoft microsoft locked as resolved and limited conversation to collaborators Dec 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants