slab / parchment

Generalized state model for rich-text editors to interface with browser DOM
BSD 3-Clause "New" or "Revised" License
628 stars 143 forks source link

Typing improvements & fix all ts errors in tests & migrate tests to vitest + playwright #132

Closed fnlctrl closed 1 year ago

fnlctrl commented 1 year ago

Sorry for yet another giant pull request, feel free to make edits directly to it.

The original goal was to fix all typing issues in tests, but I got tired of adding as SomeBlot for every .create() call, so I implemented type inference to scroll/registry to fix these once and for all.

Major changes

  1. Scroll and Registry are now parametric with Blots and Attributors
  2. Registry constructor allows passing Blots/Attributors directly. This allows statically defining typing information for variable modules, while dynamic.register() calls can't.
  3. Blots' blotName field are addded with readonly modifier to allow TS to narrow down the string type to literal type. Attributors are also made parametric (added AttrName type parameter)

These change allows intuitive type inference for most .create() and also .query() calls

Examples:

Chores

  1. TS upgraded to 5.0 along with api-extractor, enabled verbatimModuleSyntax

  2. Type-checking is added to test folder as well

  3. Added assertValidStyleKey for StyleAttributor that verifies keyName to be valid css property

fnlctrl commented 1 year ago

@luin Can you take a look? ๐Ÿ™

luin commented 1 year ago

Will do that, thanks!

fnlctrl commented 1 year ago

Continuing the discssuion for Quill's module registration - I think for starters we can decouple Attributors/Blots from Quill.register(). Currently Quill's constructor already accepts a registry option (https://github.com/quilljs/quill/blob/develop/core/quill.ts#L174-L176) - This already allows the users to construct a registry with static Attributors/Blots and pass it in. For example:

const registry = new Registry([
    ListItem,
    ListContainer,
    CodeBlock,
    CodeBlockContainer
])
const quill = new Quill({ registry })

Or have list/code block export the blots list:

import { ListItemBlots, CodeBlockBlots } from 'quill'
const registry = new Registry([
    ...ListItemBlots,
    ...CodeBlockBlots 
])
const quill = new Quill({ registry })

I think historically Quill was invented before the introduction of ES Modules so it had to implement its own module system. But since now most people are using ESM, we can drop most of quill's register/retrival logic (Quill.register(), quill.getModule())

and it would also help with tree shaking.

For core modules (clipboard, history, keyboard, uploader) they can be directly bound to the quill instance.

For custom modules, we can have a minimal plugin api that exposes the quill instance

Take modules/syntax for example, it can be used as

import { SyntaxBlots, SyntaxPlugin } from 'quill'
const registry = new Registry([ ...SyntaxBlots ])
const quill = new Quill({ registry })
quill.use(SyntaxPlugin, opts)

// scratch implementation
function SyntaxPlugin(quill, options) {
     // bind events, manipulate dom, add keyboard shortcuts, etc.
    quill.root.addEventListener(...)
    quill.container.append(...)
    quill.keyboard.addBindings({ ... })
}

Even without a plugin/module api, people have just been using the quill instance directly, e.g. y-quill which completely bypasses quill's module system.

fnlctrl commented 1 year ago

@luin Thanks for the review! All fixed - I changed .style[xxx] to .setProperty(xxx) since it doesn't seem like a breaking change. Let me know if there're anything else that needs to be changed.

luin commented 1 year ago

I believe there is difference between the setter and setProperty():

CleanShot 2023-07-04 at 14 01 56@2x
fnlctrl commented 1 year ago

@luin yeah just discovered that ๐Ÿ˜‚ It's fixed but I could revert it to reduce the scope though.

luin commented 1 year ago

Thanks! Code wise looks good to me! However, running npm test locally I got:

Failed to load url /base/test/unit/block.ts?d0e722ed289592be1ba8b32a14e42cb30f7db09d (resolved id: /base/test/unit/block.ts?d0e722ed289592be1ba8b32a14e42cb30f7db09d). Does the file exist?
Failed to load url /base/test/unit/blot.ts?0d57dd17811f3ae2df0328fc49c6c0e591a54921 (resolved id: /base/test/unit/blot.ts?0d57dd17811f3ae2df0328fc49c6c0e591a54921). Does the file exist?
Failed to load url /base/test/unit/container.ts?3198631250230f0cc5ccabb2004dfe21b336cf6c (resolved id: /base/test/unit/container.ts?3198631250230f0cc5ccabb2004dfe21b336cf6c). Does the file exist?
Failed to load url /base/test/unit/embed.ts?2a8a122495038b669bf5341e5eadf42d6c114140 (resolved id: /base/test/unit/embed.ts?2a8a122495038b669bf5341e5eadf42d6c114140). Does the file exist?
Failed to load url /base/test/unit/inline.ts?89387628f624215d6485a3bb9609ed3e03dcea10 (resolved id: /base/test/unit/inline.ts?89387628f624215d6485a3bb9609ed3e03dcea10). Does the file exist?
Failed to load url /base/test/unit/lifecycle.ts?121ec7e286299663d0fae97c036a53d109297122 (resolved id: /base/test/unit/lifecycle.ts?121ec7e286299663d0fae97c036a53d109297122). Does the file exist?
Failed to load url /base/test/unit/linked-list.ts?1ff52c70b702c5ba1aedba6eac1da19fb7c4353c (resolved id: /base/test/unit/linked-list.ts?1ff52c70b702c5ba1aedba6eac1da19fb7c4353c). Does the file exist?

Any idea why this happened?

fnlctrl commented 1 year ago

@luin Happened to me as well - I thought it was a windows issue so ignored it๐Ÿ˜‚ I'll do some testing to figure out why

fnlctrl commented 1 year ago

@luin I ended up removing karma altogether and use vitest. Seems previously I was stuck on the default webdriverio provider not working - switching to playwright was smooth and easy.

luin commented 1 year ago

Superb!