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

Fix bind type definition #75

Closed

Conversation

dustinc
Copy link

@dustinc dustinc commented Apr 22, 2022

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

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

dustinc commented Apr 26, 2022

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

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

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

@flekschas flekschas closed this May 2, 2022
@dustinc
Copy link
Author

dustinc commented May 2, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type definition for bind seems incorrect
2 participants