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

Improvement: Use string union for Block Type instead of string #83

Closed
that-ambuj opened this issue May 14, 2023 · 8 comments
Closed

Improvement: Use string union for Block Type instead of string #83

that-ambuj opened this issue May 14, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@that-ambuj
Copy link
Contributor

Hi, I have noticed that the setCustomTransformer method takes the first argument as a string for the type of block to use the transformer function(the second argument) which can be anything arbitrary. However there can be spelling mistakes by the users of this library and in order leverage typescript's type safety and tsserver's intellisense, that we use a string union for the first argument of the setCustomTransformer so that the users can get good autocompletion.

The code could look like this:

type BlockType = "image" | "video" | "file" | "pdf" | .... | string

(I've added the last union as a string so that there is flexibility to use any arbitrary string at the risk of the user of this library while also providing intellisense to the users of the library who would otherwise have to guess what string to put here.)

and we could change the type definition for setCustomTransformer to:

setCustomTransformer(type: BlockType, ...): ...
@that-ambuj
Copy link
Contributor Author

Note that this string union can also be used for the type key in BlockObjectResponse type and that'll reduce a lot of redundant code in that type and make it easier to use and understand.

@souvikinator
Copy link
Owner

souvikinator commented May 19, 2023

That's something I have faced. I was wondering if we can use existing types from notion SDK for this.
I tried looking up for it but didn't find any specific type as such.

@souvikinator souvikinator added the enhancement New feature or request label May 19, 2023
@that-ambuj
Copy link
Contributor Author

Hi, I also looked into the official notion SDK for the types but unfortunately, there isn't any such types in their library. I guess, we'll have to do this by hand.

@souvikinator
Copy link
Owner

Got done with most of it and should be fine for now. Feel free to add if you find some missing. Should be live in the patch release.

@souvikinator
Copy link
Owner

souvikinator commented May 20, 2023

So it seems like the IntelliSense is not working. Most probably because of the generic type string. Adding string & {} does the work.

@souvikinator
Copy link
Owner

Alright we are good to go ig. So I'll publish a new patch v3.0.1.

@that-ambuj
Copy link
Contributor Author

Hi, thanks for implementing this one, I thought I was going to do the PR stuff but I see you've already done it. Now that I think of it, allowing users to put any random string without any warning is a little risky and I think it's up to you if you want to allow this behaviour but I'm glad that people will get good intellisense thanks to you!

@souvikinator
Copy link
Owner

souvikinator commented May 21, 2023

I think we'll be fine and incase of any problem, always ready to fix 🧑‍🔧

but I'm glad that people will get good intellisense thanks to you!

Thanks to you :)

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

No branches or pull requests

2 participants