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 TypeScript usage examples to the docs #658

Merged
merged 1 commit into from
Jan 7, 2024

Conversation

zaaakher
Copy link
Contributor

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 davidjerleke added the documentation Improvements or additions to documentation label Dec 23, 2023
@davidjerleke
Copy link
Owner

davidjerleke commented Dec 23, 2023

Note

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

@davidjerleke davidjerleke changed the title Create typescript.mdx Add TypeScript usage examples to docs Dec 23, 2023
@davidjerleke davidjerleke changed the title Add TypeScript usage examples to docs Add TypeScript usage examples to the docs Dec 23, 2023
@zaaakher
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Owner

davidjerleke commented Dec 30, 2023

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

  • Install embla-carousel-react.
  • Expand the node_modules folder.
  • Scroll down to e and see both embla packages there: embla-carousel-react and embla-carousel.

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
Copy link
Contributor Author

zaaakher commented Dec 31, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

zaaakher commented Dec 31, 2023

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
Copy link
Owner

@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
Copy link
Contributor Author

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
Copy link
Owner

davidjerleke commented Dec 31, 2023

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
Copy link
Contributor Author

zaaakher commented Dec 31, 2023

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
Copy link
Owner

@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
Copy link
Contributor Author

zaaakher commented Jan 1, 2024

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
Copy link

mulfyx commented Jan 4, 2024

Workaround:

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

type EmblaCarouselType = NonNullable<UseEmblaCarouselType[1]>;

@davidjerleke
Copy link
Owner

davidjerleke commented Jan 4, 2024

@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
Copy link
Contributor Author

zaaakher commented Jan 5, 2024

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

Co-authored-by: David Jerleke <david.jerleke@gmail.com>
Co-authored-by: Zakher Masri <46135573+zaaakher@users.noreply.github.com>
@davidjerleke davidjerleke removed their assignment Jan 7, 2024
@davidjerleke davidjerleke added the resolved This issue is resolved label Jan 7, 2024
@davidjerleke davidjerleke merged commit 4d1b207 into davidjerleke:master Jan 7, 2024
@davidjerleke
Copy link
Owner

@zaaakher again, thanks for all your contributions and making this library better 🌟!

davidjerleke added a commit that referenced this pull request Jan 7, 2024
@zaaakher
Copy link
Contributor Author

zaaakher commented Jan 7, 2024

@davidjerleke You're very welcome brother 🌹

@mulfyx
Copy link

mulfyx commented Jan 8, 2024

i think you miss something

image

@zaaakher
Copy link
Contributor Author

zaaakher commented Jan 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation resolved This issue is resolved
Projects
None yet
3 participants