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

TSC fails to import types into non-ECMAScript module projects #1050

Closed
marksmccann opened this issue Jun 15, 2024 · 8 comments
Closed

TSC fails to import types into non-ECMAScript module projects #1050

marksmccann opened this issue Jun 15, 2024 · 8 comments

Comments

@marksmccann
Copy link

marksmccann commented Jun 15, 2024

Describe the bug
When react-imask is imported into a project that does not explicitly support ECMAScript modules via the "type" property, the TSC error ts(1479) is triggered and the types do not load.

Screenshot 2024-06-15 at 10 35 33 AM

To Reproduce
Install v7.6.1 of react-imask into a CommonJS project (either by setting "type" to "commonjs" or by omitting the property altogether) and attempt to import the dependency.

Expected behavior
The imask packages should support both CommonJS and ECMAScript modules. This can be done by omitting the "type" property from the package.json. As far as I can tell, this is what all packages intended for browser consumption do. I have yet to find an exception to this rule. For example, popular packages like "react" and "@testing-library/dom" omit the "type" property, I assume, for the aforementioned reasons. FWIW, this is also what I have done for packages that I've built and distributed.

Environment:

  • OS: N/A
  • Browser: N/A
  • Version: N/A
  • IMask version: v7.6.1
  • Framework/plugin version: N/A

Additional context
FYI, as one might assume, this is a blocker for our team. We will not be able to upgrade to v7 until this is resolved.

@uNmAnNeR
Copy link
Owner

hi, sorry for long delay.
"type": "module" is used to be able to use es-modules for build and test purposes.
To support commonjs there is an "exports" property. It should work well with modern toolchains.
Feel free to reopen if you have more context to share but in general i have no idea how to deal with this.

@marksmccann
Copy link
Author

Thanks for the reply. Unfortunately, I can't let this one go. If this is not resolved, our team will not be able to upgrade to the latest major version – unless we want to lose all the benefits of TypeScript. I'll put together a PR with some suggested changes that would resolve this issue. We can continue the conversation over there.

@marksmccann
Copy link
Author

I do not seem to have authority to reopen the issue (which makes sense). Please reopen this issue pending my PR.

marksmccann pushed a commit to marksmccann/imaskjs that referenced this issue Oct 13, 2024
@alexandersorokin
Copy link

alexandersorokin commented Nov 1, 2024

In order to work correctly, there should be separate types for esm and cjs bundles. Something like that:

"exports": {
  "import": {
    "types": "./esm/index.d.ts",
    "default": "./esm/index.js",
  },
  "require": {
    "types": "./cjs/index.d.cts",
    "default": "./cjs/index.cjs",
  }
}

There is no need to remove "type": "module".

Also, moduleResolution should be Node16. node value is legacy and doesn't support ES Modules.

You can check types by arethetypeswrong utility here: https://arethetypeswrong.github.io
Arethetypeswrong utility is provided by TypeScript core contributor.

@marksmccann
Copy link
Author

Thank you for sharing a link to that tool. It validates my point. The types are not being resolved properly for imask or react-imask. My PR #1083 does fix this issue.

Screenshot 2024-11-01 at 8 26 41 AM

@alexandersorokin
Copy link

alexandersorokin commented Nov 1, 2024

Thank you for sharing a link to that tool. It validates my point. The types are not being resolved properly for imask or react-imask. My PR #1083 does fix this issue.

Yes, it shows that there are issue with packages. But the removing "type": "module" is not a good solution. It's downgrade by various reasons. For example, removing means downgrading from esm to commomjs.

A good (and recommended by TypeScript authors) solution is specifying moduleResolution: Node16 and building types simultaneously with js-files twice: for commonjs and for esm. And then choosing between versions using exports field value I provided.

You can also do not specify "types" in "exports" field. TypeScript find types automatically if *.d.ts files reside alongside with .js files in the same folder. For example it works:

"exports": {
  "import":  "./esm/index.js",
  "require": "./cjs/index.cjs"
}

With file structure:

./esm/index.d.ts
./esm/index.js
./cjs/index.d.cts
./cjs/index.cjs

To get rid of cjs-files we can add empty package.json to commonjs folder:

./package.json // with `type: module` and exports
./esm/index.d.ts
./esm/index.js
./cjs/index.d.ts
./cjs/index.js
./cjs/package.json //empty

But having separate d.ts files for commonjs and esm versions is only supported solution that works everywhere (for example in SSR).

@marksmccann
Copy link
Author

Fair enough. If there is a better way to resolve the issue, then great! But, can we now agree that this is a valid problem that needs to be resolved? Will you reopen the issue?

@alexandersorokin
Copy link

@uNmAnNeR it is still a issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants