flekschas / svelte-simple-modal

A simple, small, and content-agnostic modal for Svelte v3 and v4
https://svelte.dev/repl/b95ce66b0ef34064a34afc5c0249f313
MIT License
422 stars 30 forks source link

Fix bind type definition #75

Closed dustinc closed 2 years ago

dustinc commented 2 years ago

Fixes #70

The problem seems to be with trying to use sveld for type generation outside a svelte component, i.e., the bind function in the "module" script at the top of src/Modal.svelte

The issue is twofold.

First, sveld is creating the bind function's type as, well, a type:

Screen Shot 2022-04-22 at 11 42 27 AM

When what we need is a function:

Screen Shot 2022-04-22 at 11 45 20 AM

Second, the the default export from Modal.svelte is being set as both bind and Modal.

Screen Shot 2022-04-22 at 11 47 56 AM

This is what causes the "Value of type 'typeof Modal' is not callable" IDE error.

Screen Shot 2022-04-22 at 11 56 24 AM

When what we want is separate export for bind:

Screen Shot 2022-04-22 at 11 52 18 AM

Please review the changes and ask any questions. If there is a way to accomplish this with only sveld then please disregard and advise.

Thanks!

flekschas commented 2 years ago

Thanks for putting this together. Ideally I would not like to make the types compilation process even more complicated and use sveld exclusively.

To clarify, is the first issue really an issue? I am not a TS expert, so I am curious what the issue is with declaring bind as a type?

The second issue seems to be the real problem. I wonder if we can open an issue with sveld to have them look into it. I've done this before and they were pretty responsive at fixing the problem.

dustinc commented 2 years ago

Yeah, I totally get wanting to use sveld for everything. Maybe that is something they are willing to do.

Both issues are about equal, actually. Because once the type error mentioned in the 2nd issue is resolved, you'll get one like this due to bind being a type instead of a function as I previously mentioned:

Screen Shot 2022-04-26 at 12 22 23 PM

Either way, these "error" messages are kinda superficial. Meaning, the bind function itself exists properly in the compiled js, so the code works, but vscode/lint complains. We're basically just throwing the usefulness of TS out the window, though.

Sounds like the purpose of sveld is for svelte component-to-type generation only. Since they're already using tsc to do this (which is what I'm using in the PR) maybe they will expand their functionality.

For me, since time is of more importance than type for my particular use, simply importing directly from svelte-simple-modal/lib works fine. I'm willing to forego TS handiness for this particular function. But, outside of asking sveld to expand their functionality for this type of use, my proposed solution works... mostly because that is just how types are generated. Not to say it's the best/prettiest, though!!

Thanks for reviewing, though! :)

flekschas commented 2 years ago

Thanks for the detailed clarifications!

I was mostly thinking in the long-term and how much maintenance a custom work-around will add over time. TS adds some handiness but I sometimes wondering if the overhead is really worth it in terms of preventing bugs from happening... Having said that, let me test your solution locally and if it works we can merge it for now. I'll also open a ticket with sveld in the hopes that they will add built-in support for this. I am sure others will eventually run into similar issues.

I am pretty busy at the moment with other stuff but I hope to get to it this evening or in the next couple of days.

Thanks again for taking the time and putting together the PR! It's definitely much appreciated 🙏

flekschas commented 2 years ago

@dustinc Thank you so much again for putting this PR together and the detailed explanation. I realized the second issue can be fixed by changing the export statement of index.ts. And after having reached out to the folks behind sveld, they fixed the function declaration issue with hours.

Hence, I unfortunately will have to close this PR as it's been superseded. The just released v1.3.2 fix the type issue.

dustinc commented 2 years ago

@flekschas That is great to hear!!! Major kudos to sveld on this one!!