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

Type for ContextRegistry #30

Open
boris-petrov opened this issue Sep 2, 2024 · 5 comments · May be fixed by #32
Open

Type for ContextRegistry #30

boris-petrov opened this issue Sep 2, 2024 · 5 comments · May be fixed by #32

Comments

@boris-petrov
Copy link
Contributor

I believe the type for ContextRegistry could be improved by removing the index signature. Leaving it means that no matter what I write in (for example) ContextProvider @key - Glint won't complain. If the index signature is not there, only valid keys will be accepted. Which is a much better developer experience. What do you think?

@kevinkucharczyk
Copy link
Collaborator

@boris-petrov I think that suggestion makes sense!
Restricting the provider component (and decorator) to only accept explicitly declared keys might help developers catch certain bugs early by warning them when they're trying to use unknown context keys.

The registry is defined the way it is now to make it easier for developers to adopt this addon, though as it matures I can definitely see us switching to more strict types.

@boris-petrov boris-petrov linked a pull request Sep 9, 2024 that will close this issue
@boris-petrov
Copy link
Contributor Author

Thanks for the answer! I've opened a PR for this. I'm not sure how this index-signature makes it easier to adopt the addon - it only would allow people to make typos that are not caught. Maybe I'm missing something - if you think it's not the right time, you can close the PR or leave it open for later. :)

@kevinkucharczyk
Copy link
Collaborator

I'm not sure how this index-signature makes it easier to adopt the addon - it only would allow people to make typos that are not caught

The way it's currently typed makes it easier because a developer doesn't have to add keys to the context registry to use this addon (without typescript complaints).

It's a minor thing, though it does lower the barrier of adopting the addon a little bit. But I agree with you that this is the less type-safe way to do it, as you don't get any hints on whether the keys you're adding are in fact correct.

@boris-petrov
Copy link
Contributor Author

Ah, I see what you mean. Well, I would argue that if one is using JS only that's not a problem. If one is using TS, they would certainly want more type-safety anyway. But you're technically right so I'll leave the decision whether to merge the commit to you. :)

@arthur5005
Copy link

fwiw, I think this is a good idea, and would love to see the above mentioned PR merged :)

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 a pull request may close this issue.

3 participants