davidjerleke / embla-carousel

A lightweight carousel library with fluid motion and great swipe precision.
https://www.embla-carousel.com
MIT License
5.39k stars 166 forks source link

Add TypeScript usage examples to the docs #658

Closed zaaakher closed 5 months ago

zaaakher commented 6 months ago

Added typescript page for the docs


This is just a starting point since I had to wing it since I couldn't get the gatspy docs running locally in my PC. I will add more when I get the chance god willing

davidjerleke commented 6 months ago

[!NOTE] We need to add this to the docs and also update all react CodeSandbox examples with the same approach.

zaaakher commented 6 months ago

@davidjerleke I finalized the typescript page and also updated the way types are imported in the Sandboxes.

Kindly take a look at it and let me know you need additional help šŸ‘

zaaakher commented 6 months ago

Hey @davidjerleke ,

I've been working on my UI kit library here and I have the carousel component working well with only using embla-carousel-react package. Now that the types will move into embla-carousel package, does that mean I have to install it just to use the types? would it be bad to install embla-carousel as a devDependency?

Or should we look into making a PR for creating a @types/embla-carousel package in this repo?

Or would it be strategic to make a package solely for the types? like embla-carousel-types ?

Based on your comment here , embla-carousel should be a dependency but somehow I got the carousel working without embla-carousel and with only the react wrapper (here's my package.json)

davidjerleke commented 6 months ago

@zaaakher embla-carousel-react has embla-carousel as a dependency, so when you install the react package it will automatically install the core package under the hood. You can easily confirm this like so:

So you should import types from the core package embla-carousel. I will remove any type re-exports from all library/framework packages in the next release. This might be the reason itā€™s working for you now but some devs were experiencing problems with the type re-exports in the library/framework wrappers.

I donā€™t think itā€™s common practice to make a separate types package because Embla has first class TypeScript support. Itā€™s built with TypeScript. I think type packages are mainly used for packages built without TypeScript.

I hope my explanation makes sense?

Best, David

zaaakher commented 6 months ago

I donā€™t think itā€™s common practice to make a separate types package because Embla has first class TypeScript support. Itā€™s built with TypeScript. I think type packages are mainly used for packages built without TypeScript.

True true, I agree.


embla-carousel-react has embla-carousel as a dependency, so when you install the react package it will automatically install the core package under the hood

That doesn't seem to be working as expected when using pnpm workspace (and embla-carousel doesn't appear in the node_modules)

in my project here I can do as you mentioned and everything works fine.

But currently i'm migrating to a proper monorepo structure using pnpm (in the monorepo branch) and it seems embla-carousel is not recognized.

image

zaaakher commented 6 months ago

Looking at this I was able to find embla-carousel in the root node_modules inside .pnpm but doing the following won't work and doesn't even look right.

import { EmblaOptionsType } from "../../../../node_modules/.pnpm/embla-carousel@8.0.0-rc17";

It's most likely a pnpm thing but I think it could be an important caveat to include somewhere in this library.

zaaakher commented 6 months ago

shadcn-ui is using this library but they still didn't migrate to the new typescript approach. And they're using pnpm as well.

So there's a chance they might stumble upon a similar problem if not an identical one to mine when they update embla carousel as per your suggestion here and change the way they import types.

Of course a simple solution would be to explicitly install embla-carousel but I'm inclined to resist to do that.

davidjerleke commented 6 months ago

@zaaakher thatā€™s valuable information. Thank you. I havenā€™t used pnpm so didnā€™t know about this. If this is the case, the solution would be to copy over the types from the core package to the library wrappers during the build process. Are there anymore things to consider to fully support pnpm?

zaaakher commented 6 months ago

Are there anymore things to consider to fully support pnpm?

Making embla-carousel a peer dependency of embla-carousel-react could potentially solve this problem (I'm not entirely sure if it will). But would doing that cause problems elsewhere?

davidjerleke commented 6 months ago

Making embla-carousel a peer dependency of embla-carousel-react could potentially solve this problem (I'm not entirely sure if it will). But would doing that cause problems elsewhere?

@zaaakher Iā€™m not sure how that would affect npm/yarn and pnpm for that matter. Is that a conventional solution?

zaaakher commented 6 months ago

Is that a conventional solution?

It doesn't feel like it and i'm very doubtful if it'd work.

the solution would be to copy over the types from the core package to the library wrappers during the build process

That's a good solution, I can't think of a better one at the moment. Although is there a possible scenario in the future where a library wrapper uses a slightly different type from the one in the core library? (i.e. something unique to react or vue or server components)

Currently I'm using types the original way šŸ‘‡

import useEmblaCarousel, { EmblaOptionsType } from "embla-carousel-react";

I think it could be a good developer experience when consumers of this library have to install a single package (depending on their framework) where they can import hooks, types, and possibly components.

Honestly I never knew I could import from a package that's not listed in my package.json (other than the node built-in packages), I just learned I could do that from you šŸ˜„ (although that didn't work with pnpm).

I wish I have more time on my hand to test it out with pnpm alternatives like lerna, rush, or bolt. Nevertheless, I have a feeling they all have a similar approach to how they put aside dependencies-of-dependencies to save disk space and increase performance within a monorepo structure.

I understand there could be performance/size trade-offs depending on the approach you'd like to take and I'm here if you wanna bounce ideas around, I'm just thinking out loud.

davidjerleke commented 6 months ago

@zaaakher I will try the copy types approach then. Thanks for your thoughts šŸ‘šŸ».

Wow. The frontend tooling world is so broken. Itā€™s really a nightmare to try to support everything. Feels like we spend 95% of our time with toolsā€¦

zaaakher commented 6 months ago

Wow. The frontend tooling world is so broken. Itā€™s really a nightmare to try to support everything. Feels like we spend 95% of our time with toolsā€¦

True. It's the same cycle where an individual or a group of people make tool, then it gains popularity, then it's a either they have to start adapting to others or others have to adapt to them.

mulfyx commented 6 months ago

Workaround:

import { UseEmblaCarouselType } from "embla-carousel-react";

type EmblaCarouselType = NonNullable<UseEmblaCarouselType[1]>;
davidjerleke commented 6 months ago

@zaaakher Iā€™ve been considering my options and I havenā€™t gotten anywhere to be honest.

Copying types

Copying the types from the core library wonā€™t work if someone build a wrapper or plugin that resides in a separate repository. So a requirement for this approach is that every Embla official package has to be in this repository.

Separate type packages

One solution is to create a separate package @types/<package-name> for every Embla package. This is of course doable but will take some effort and it will not be as convenient for the end user.

Add core library to peer dependencies

This is how the plugins do. They only need types from the core package and not modules, so they have the core package as a peer dependency. And it seems like pnpm doesnā€™t isolate packages listed under peer dependencies like it does with packages under dependencies. I donā€™t know if npm or yarn or pnpm will throw warnings for this but maybe itā€™s worth to at least test this because this is easy to do. If I test npm and yarn, could you help me with how to test pnpm? Or maybe it should be under devDependencies? Doesnā€™t that make sense?

Best, David

zaaakher commented 5 months ago

Hi @davidjerleke ,

I've been reading about pnpm and looking at other monorepo projects using pnpm and I thought I could configure pnpm to exclude embla-carousel from the automatic strict dependency resolution but to no avail.

So I think the best option for me is to install embla-carousel as a devDependency because I only need the types.

Confirmation

I tested it in my UI library kit and I installed embla-carousel as a devDependency here and I imported the types and used it here and everything worked fine.

I then published the ui kit @sikka/hawa@0.26.21 and used it in a new next.js app just for testing and everything worked fine I got autocomplete of the types which are coming from EmblaOptionsType.

Final thoughts

embla-carousel is technically already installed behind the scene because embla-carousel-react depends on it but pnpm automatically hides it in a /node_modules/.pnpm and makes it inaccessible to me to import the types from. So installing embla-carousel as a devDependency makes the most sense because I only need it during development.

Although my tsup build process took the types and bundled it with the package that's why it enabled autocomplete to the consumer of @sikka/hawa

So I think a caveat we could mention in the docs is to install embla-carousel as a devDependency when a wrapper library like embla-carousel-react is used in a pnpm-managed monorepo.

I hope that was helpful šŸ‘

Thanks, Zakher

davidjerleke commented 5 months ago

@zaaakher again, thanks for all your contributions and making this library better šŸŒŸ!

zaaakher commented 5 months ago

@davidjerleke You're very welcome brother šŸŒ¹

mulfyx commented 5 months ago

i think you miss something

image

zaaakher commented 5 months ago

Yup @mulfyx , it'll be fixed with #685