Migration to Typescript #183
Conversation
|
@marijnh I hope you don't mind me pinging you here. I know you are not maintaining this project. However I am in the progress of migrating this package from JS to TS, like you did recently all other prosemirror packages. And I meant to re-use your build tools to keep everything consistent. But I might need guidance on how you use them, and whether there are a good fit for this package too ? First I am a bit confused by the absence of Then And finally what is your development workflow ? I could not find any "watch" options in your tools. Are you using PS: @ocavue, the conversion should be almost done. The only things missing are a solid build system and enabling |
|
See the readme in https://github.com/prosemirror/prosemirror for my multi-package setup. But since this is maintained separately, you'll probably want to use a different setup for it. |
PowerKiKi
force-pushed
the
typescript
branch
2 times, most recently
from
e3521c4 to
d262939
Compare
This does not use the official prosemirror build chain, but instead keep our custom build chain based on Rollup. While the commit is big I tried to minimze changes as much as possible. Specifically no logic should have been touched at all. Typing is now generated by typescript and bundled by api-extractor, so that is always synced.
PowerKiKi
force-pushed
the
typescript
branch
from
d262939 to
700e96a
Compare
|
This PR is ready for review. @ocavue could please approve the CI, so that we can be sure that everything is fine ? |
| /** | ||
| * @public | ||
| */ | ||
| export function addColSpan(attrs: Attrs, pos: number, n = 1): Attrs { |
There was a problem hiding this comment.
The old addColSpan and removeColSpan has <T>. Maybe we should add <T> too to avoid breaking changes?
There was a problem hiding this comment.
I always end up with a typescript error down the line when I try to add generic. I am not entirely convinced whether we need a generic or not here 🤔
Would you have a use-case to help convincing me ?
There was a problem hiding this comment.
To be honest, I don't feel the generic here is much useful. I only reason I posted my previous comment is to avoid breaking change. To make the code clear, I'm ok to remove the generic here. We have such breaking changes during the migration from @types/prosemirror-* to official prosemirror-* packages too, so I guess it's not a big deal.
|
@PowerKiKi Thank you so much for your contribution! I'm sorry that it takes me so long to find a time reviewing it. I posted some review comments above. Let me know what do you think about them. |
Use a `Record`, instead of a literal, to be compatible with `OrderedMap.append()`
Co-authored-by: ocavue <ocavue@users.noreply.github.com>
|
I fixed everything, except the last point. Maybe you can have a look again... |
This does not use the official prosemirror build chain, but instead keep our custom build chain based on Rollup.
While the commit is big I tried to minimize changes as much as possible. Specifically no logic should have been touched at all.
Typing is now generated by typescript and bundled by api-extractor, so that is always synced.