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

chronoDave commented 1 year ago

Bug is related to

[x] embla-carousel (core package)
[x] embla-carousel-react
[x] embla-carousel-vue
[x] embla-carousel-svelte
[x] embla-carousel-autoplay
[x] embla-carousel-auto-height
[x] 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

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:

davidjerleke commented 1 year ago

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

Best, David

chronoDave commented 1 year ago

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 commented 1 year ago

@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

chronoDave commented 1 year ago

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:

https://github.com/ezolenko/rollup-plugin-typescript2/issues/437

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 commented 1 year ago

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 commented 1 year ago

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

https://github.com/davidjerleke/embla-carousel/assets/27073716/0b4e01e1-0859-456e-ae22-a4969b25e2ed

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 :)

davidjerleke commented 1 year ago

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 commented 1 year ago

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).

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

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

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.

chiptus commented 10 months ago

after reading https://github.com/microsoft/TypeScript/issues/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 commented 8 months ago

@chronoDave never mind. I found a solution!

davidjerleke commented 8 months ago

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

davidjerleke / embla-carousel