search

souvikinator / notion-to-md

Convert notion pages, block and list of blocks to markdown (supports nesting and custom parsing)
https://www.npmjs.com/package/notion-to-md
MIT License
1.08k stars 89 forks source link

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

Closed that-ambuj closed 1 year ago

that-ambuj commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

that-ambuj commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

souvikinator commented 1 year ago

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

that-ambuj commented 1 year ago

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 commented 1 year ago

I think we'll be fine and incase of any problem, always ready to fix :mechanic:

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

Thanks to you :)