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

Add "type: module" (nodenext) support alongside commonjs #481

Closed
7 tasks done
Tracked by #321
chronoDave opened this issue May 25, 2023 · 11 comments · Fixed by #605
Closed
7 tasks done
Tracked by #321

Add "type: module" (nodenext) support alongside commonjs #481

chronoDave opened this issue May 25, 2023 · 11 comments · Fixed by #605
Labels
feature request New feature or request resolved This issue is resolved typescript Issue is related to TypeScript

Comments

@chronoDave
Copy link

chronoDave commented May 25, 2023

Bug is related to

  • embla-carousel (core package)
  • embla-carousel-react
  • embla-carousel-vue
  • embla-carousel-svelte
  • embla-carousel-autoplay
  • embla-carousel-auto-height
  • embla-carousel-class-names

Embla Carousel version

  • 8.0.0-rc05

Describe the bug

When using node16 esm import in TypeScript, TypeScript with incorrectly flag up the embla-carousel default import as missing. This is due to incorrect typing: https://arethetypeswrong.github.io/?p=embla-carousel%408.0.0-rc05

image

This error can be ignored using @ts-expect-error or something similar as I can confirm that the default import does work, it's just the typing that's incorrect.

CodeSandbox

Unfortutately this bug does not occur when usign CodeSandbox. If needed, I can create a simple repository demonstrating this behaviour.

package.json

{
  "name": "nodenext",
  "type": "module",
  "dependencies": {
    "embla-carousel": "^8.0.0-rc03",
    "typescript": "^5.0.4"
  }
}

tsconfig.json

{
  "compilerOptions": {
    "module": "NodeNext",
  }
}

index.ts

import EmblaCarousel from 'embla-carousel';

const carousel = EmblaCarousel();

Steps to reproduce

Simply hover over the EmblaCarousel error in your IDE of choice (mine being VSC)

Expected behavior

I expect the default export to be typed correctly.

Additional context

It seems to be a common issue:

@chronoDave chronoDave added the bug Something isn't working label May 25, 2023
@davidjerleke
Copy link
Owner

Thanks @chronoDave. Do you happen to know how to solve this?

Best,
David

@chronoDave
Copy link
Author

I've not done much research yet but I can pick it up if you'd like. I suspect it would be simply following the advice from https://arethetypeswrong.github.io/?p=embla-carousel%408.0.0-rc05

The types resolved at the package use export default where the implementation appears to use module.exports =. Node treats a default import of these constructs from an ES module differently, so these types will make TypeScript under the node16 resolution mode think an extra .default property access is required, but that will likely fail at runtime in Node. These types should use export = instead of export default.

Though I'm not sure how it would impact cjs imports.

@davidjerleke
Copy link
Owner

davidjerleke commented May 25, 2023

@chronoDave seems like this could do the trick but the main part of investigating this would be to make it compatible with all and how to generate this automatically. Because I use rollup with TypeScript to create all the bundles and types. At the time of writing I'm generating cjs, esm and umd bundles.

Feel free to investigate this further if you want.

Best,
David

@davidjerleke davidjerleke added the typescript Issue is related to TypeScript label May 25, 2023
@chronoDave
Copy link
Author

Thank you for the slugify recommendation. You're correct in that it's probably not in the best interest to monkey-patch the index.d.ts file produced by rollup.

After doing some more research into how rollup creates index.d.ts, it seems the issue is with rollup-plugin-typescript2. It currently does not support node16:

I've tried solving the issue using @rollup/plugin-typescript but had no luck getting it to export correctly.

I feel it's probably best to wait for that issue to be resolved and then see if that solves the problem.

@davidjerleke
Copy link
Owner

Thanks for taking the time to investigate this @chronoDave 👍.

At the time of writing my understanding of this problem and possible solutions is very limited. So sorry if this is a stupid question: What's your understanding of this problem? Because I'm a bit confused. The solution seems to be changing:

// from:
export default EmblaCarousel

// to:
export = EmblaCarousel

But is this backwards compatible for esm with node versions < 16? Or does this mean that we need to create two esm bundles with both solutions and point to different esm files (in package.json or similar) depending on the node version used?

Best,
David

@chronoDave
Copy link
Author

chronoDave commented May 26, 2023

The issue lies with TypeScript not being able to tell the difference between a cjs and a esm package. Even though you're exposing both esm and cjs packages, the index.d.ts file is only valid for cjs, therefore causing TypeScript to import the esm module as a cjs module and throwing the "has no call signatures" error. This is also why it's a typing error and not a code error. The esm code is valid, the index.d.ts is not.

I've attached a video that should hopefully better describe the issue. I've created a repo as well so you can easily verify it for yourself: https://github.com/chronoDave/ts-esm-cjs

Code_PhvzKXuo8y.mp4

When trying to resolve the embla-carousel import, the following steps happen:

Types

  1. It looks for a type declaration, which it finds based on types: index.d.ts
  2. There's no type field, so it defaults to cjs
  3. TypeScript throws an error because the embla-carousel.esm.js bundle does not expose correct cjs (as it's esm)

Code

  1. It looks for an esm bundle, which it finds based on module: embla-carousel.esm.js
  2. It's valid esm so it runs correctly!

By adding type: module, it makes it explicit that the types: index.d.ts file is esm. However, the index.d.ts file does not have valid esm as it's missing file extensions (as seen in the video).


As for a solution, creating seperate type declarations seems to do the trick, but this requires type: module in the package.json to work. It seems to be working for moduleResolution: node and moduleResoltion: nodenext but I'd have to do more testing to see what impact this solution would have.

  "exports": {
    ".": {
      "import": {
        "default": "./embla-carousel.esm.js",
        "types": "./esm.d.ts"
      },
      "require": {
        "default": "./embla-carousel.cjs.js",
        "types": "./cjs.d.ts"
      }
    }
  },

I hope this helps clarify the issue more. My apologies if my answers seem vague, I'm figuring it out as I go, I don't have too much experience yet with using moduleResolution: nodenext :)

@chronoDave chronoDave changed the title Default import not typed correctly when using "type: module" and "module: NodeNext" Default import not typed correctly when using "type: module" and "moduleResolution: nodenext" May 26, 2023
@davidjerleke
Copy link
Owner

davidjerleke commented Jun 8, 2023

Hi @chronoDave,

I just found this article that claims you can support both imports and require. I haven't tried it yet though. Feel free to try it out if you get the chance to do so before me.

Best,
David

@chronoDave
Copy link
Author

Hey @davidjerleke,

I've verified the solution mentioned in the article you linked and it surprisingly works! I've moved the CJS and ESM exports into their respective folders and duplicated the index.d.ts in those folders as well (as seen in the screenshot) and then tried importing embla-carousel using "moduleResolution": "nodenext" (esm) and "moduleResoltion": "node" (cjs).

image

The main package.json has the added exports field as shown in the article

image

The package.json in the cjs folder simply contains { "type": "commonjs" } and the package.json in the esm folder contains { "type": "module" }

image

image

I'm pretty confident it's possible to generate the folder structure in this way using rollup. If you'd like I can create a PR with my ideas.

@davidjerleke davidjerleke mentioned this issue Jul 12, 2023
37 tasks
@chiptus
Copy link

chiptus commented Aug 20, 2023

after reading microsoft/TypeScript#49189 I got a workaround for now:

import _useEmblaCarousel, {EmblaOptionsType, EmblaPluginType, UseEmblaCarouselType} from "embla-carousel-react";

const useEmblaCarousel = _useEmblaCarousel as unknown as (options?: EmblaOptionsType, plugins?: EmblaPluginType[]) => UseEmblaCarouselType;

@davidjerleke
Copy link
Owner

davidjerleke commented Oct 19, 2023

@chronoDave never mind. I found a solution!

@davidjerleke davidjerleke changed the title Default import not typed correctly when using "type: module" and "moduleResolution: nodenext" Add "type: module" (nodenext) support alongside commonjs Oct 23, 2023
@davidjerleke davidjerleke added feature request New feature or request and removed bug Something isn't working labels Oct 23, 2023
davidjerleke added a commit that referenced this issue Oct 31, 2023
davidjerleke added a commit that referenced this issue Oct 31, 2023
davidjerleke added a commit that referenced this issue Oct 31, 2023
Add "type: module" (nodenext) support alongside commonjs
@davidjerleke davidjerleke added the resolved This issue is resolved label Nov 1, 2023
@davidjerleke
Copy link
Owner

@chronoDave and @chiptus this will be released with the next version of Embla.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request resolved This issue is resolved typescript Issue is related to TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants