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

[Bug]: TypeError/regression introduced in 2.10.1: All options can be undefined #5859

Open
1 task done
Mathias-S opened this issue Nov 22, 2024 · 16 comments
Open
1 task done
Labels
Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug

Comments

@Mathias-S
Copy link
Contributor

Mathias-S commented Nov 22, 2024

Affected Packages

react, core, all extensions

Version(s)

2.10.1

Bug Description

Code that was previously working fine, now gives a TypeError when trying to access properties on this.options. These two examples were working fine in Tiptap 2.9, but now give errors:

Example 1

import Paragraph from "@tiptap/extension-paragraph";
import { mergeAttributes } from "@tiptap/react";

const MyStrangeParagraph = Paragraph.extend({
  renderHTML({ HTMLAttributes }) {
    return [
      `h1`,
      mergeAttributes(this.options.HTMLAttributes, HTMLAttributes),
      //              ^^^^^^^^^^^^^^^^^^^^^^^^^^^ 'undefined' is not assignable to 'Record<string, any>'
      0,
    ];
  },
});

Example 2

import Link, { type LinkOptions } from "@tiptap/extension-link";

interface MyLinkOptions extends LinkOptions {
  myFunc: () => void;
}

const MyLink = Link.extend<MyLinkOptions>({
  addCommands() {
    return {
      ...this.parent?.(),
      myNewCommand: () => () => {
        this.options.myFunc();
//      ^^^^^^^^^^^^^^^^^^^ Object possibly 'undefined'
      },
    };
  },
});

MyLink.configure({ myFunc: () => undefined });

Browser Used

Chrome

Code Example URL

No response

Expected Behavior

For example 1, I would expect this to work. I'm not sure how this can be done without using a non-null assertion this.options.HTMLAttributes! or without writing extra logic that wasn't previously necessary.

For example 2, I recognise that MyLink can be configured without passing in myFunc, so maybe the old code was wrong and there should always have been a nullish check such as this.options?.myFunc(). But shouldn't it be possible to provide mandatory options?

Additional Context (Optional)

Introduced with release 2.10.1 with #5854.

Also, I understand that the options types may be more correct now, but these changes are breaking type changes and according to Renovatebot, which we are using in our project, the passing percentage for this update is quite low, indicating there have been breaking changes. I don't know if this specific type change is the breaking change for everyone, but it is for us. See the below screenshot that shows Renovatebot stats for some of the packages.

image

Dependency Updates

  • Yes, I've updated all my dependencies.
@Mathias-S Mathias-S added Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug labels Nov 22, 2024
@nperez0111
Copy link
Contributor

Well that is a nifty package.

I am torn here because the type change introduced by 2.10 actually can prevent you from accessing something that does not exist (leading to a runtime error), and then there is this which is just forcing users to do checks for something that theoretically is accurate (someone may forget to call this.parent() in their addOptions). Not sure of an in-between on here.

@Mathias-S
Copy link
Contributor Author

Mathias-S commented Nov 22, 2024

What if mergeAttributes (and possibly other similar utilities that I'm not familiar with) would allow undefined in the type signature? The implementation already handles this just fine.

export function mergeAttributes(...objects: (Record<string, any> | undefined)[]): Record<string, any> {

Then the fix on our end would be trivial (just add optional chaining when accessing properties from this.options). Still a somewhat breaking change (type wise), but one that's much easier to handle. And arguably not really breaking since old implementations would have been incorrect.

But for other users there may be other cases where the type changes in 2.10 and 2.10.1 cause issues that I'm not aware of.

@nperez0111
Copy link
Contributor

I don't know that it really is easier. I've been trying to see if I can get a typing that works but it all very complicated with this pseudo inheritance. I think it is better to just revert both changes & leave the types alone. There will always be someone who complains about types

nperez0111 added a commit that referenced this issue Nov 22, 2024
* revert: "fix(core): update the typings to be that options and storage are partials on an extended config #5852 (#5854)"

This reverts commit 87d63d8.

* revert: "fix(core): update the typing of `addOptions`, `addStorage` to have an optional parent #5768 (#5770)"

This reverts commit d2f366d.
@nperez0111
Copy link
Contributor

This was released with 2.10.2

gethari pushed a commit to gethari/tiptap that referenced this issue Nov 23, 2024
…osis#5860)

* revert: "fix(core): update the typings to be that options and storage are partials on an extended config ueberdosis#5852 (ueberdosis#5854)"

This reverts commit 87d63d8.

* revert: "fix(core): update the typing of `addOptions`, `addStorage` to have an optional parent ueberdosis#5768 (ueberdosis#5770)"

This reverts commit d2f366d.
@Mathias-S
Copy link
Contributor Author

Mathias-S commented Nov 25, 2024

@nperez0111 Just thought you'd be interested in the updated stats from Renovatebot (the Passing column) 😄

image

@nperez0111
Copy link
Contributor

Thanks @Mathias-S , where would I be able to see that information 🤔

@Mathias-S
Copy link
Contributor Author

Honestly I'm not sure. We are using Renovatebot in our (private) repository, and it works similarly to dependabot's PRs, except it groups related updates in a single PR and shows the stats I shared above. So this is what the PR looks like:

image

https://docs.renovatebot.com/merge-confidence/
https://www.mend.io/blog/merge-confidence/

I'm not sure how you can access those stats without basically setting up your own repo with Renovatebot integration and all plugins installed. There may be an API for it, but after a few minutes of searching it didn't look like it was publicly documented.

@mgreystone
Copy link
Contributor

What if mergeAttributes (and possibly other similar utilities that I'm not familiar with) would allow undefined in the type signature? The implementation already handles this just fine.

export function mergeAttributes(...objects: (Record<string, any> | undefined)[]): Record<string, any> {

Then the fix on our end would be trivial (just add optional chaining when accessing properties from this.options). Still a somewhat breaking change (type wise), but one that's much easier to handle. And arguably not really breaking since old implementations would have been incorrect.

But for other users there may be other cases where the type changes in 2.10 and 2.10.1 cause issues that I'm not aware of.

For my own education, why the hesitation for this approach? After all, it seems like the implementation already handles undefined safely. Seems like this type change would also be "more correct" & bring the types & runtime closer together?

@nperez0111
Copy link
Contributor

For my own education, why the hesitation for this approach? After all, it seems like the implementation already handles undefined safely. Seems like this type change would also be "more correct" & bring the types & runtime closer together?

Sure mergeAttributes could be more forgiving & that would be correct to it's implementation. But, it is masking over the real issue, that this.options is always defined for an .extend extension. So, making it possibly null enforces type checks everywhere it is referenced which also "breaks" types for people. This affects options & storage when they should always be available

@mgreystone
Copy link
Contributor

Okay, what about applying this change to just this.parent? My understanding is that this.parent is never guaranteed to be defined, right? Selfishly, this.parent the only one that is giving me trouble, though it would still be considered a "breaking" type change.

@nperez0111
Copy link
Contributor

this.parent is guaranteed to be defined on a .extend'd config.

It is guaranteed to be nullish on a .create'd config.

So whatever type can convey that without breaking people I'm fine with

@mgreystone
Copy link
Contributor

My apologies, but i do not understand. My underlying issue is that this.parent is not defined in an extend'd config. Am i missing something silly?

import { Heading, type Level } from '@tiptap/extension-heading'

const MyHeading = Heading.extend({
  addOptions() {
    const levels: Level[] = [1,2,3]
    return {
      ...this.parent(), // this throws even though typescript says it is safe
      levels,
    }
  }
})

@nperez0111
Copy link
Contributor

My apologies, but i do not understand. My underlying issue is that this.parent is not defined in an extend'd config. Am i missing something silly?

The type should be:

const ExistingExtension = Node.create({
 addOptions(){
   this.parent // should be null or undefined
 }
})


const ExtendedExtension = ExistingExtension.extend({
 addOptions(){
   this.parent // should be `Node` pointing to ExistingExtension's type
 }
})

This is the most accurate representation of the types. Within a .create the parent is undefined, within a .extend the parent is always defined.

@mgreystone
Copy link
Contributor

I think i'm understanding, but that is not the behavior i am seeing in 2.10.3. If i add console logs to the code above like so:

const ExistingExtension = Node.create({
 addOptions(){
   console.log('existing', this.parent)
   this.parent // should be null or undefined
 }
})

const ExtendedExtension = ExistingExtension.extend({
 addOptions(){
   console.log('extended', this.parent)
   this.parent // should be `Node` pointing to ExistingExtension's type
 }
})

const schema = getSchema([ StarterKit, ExtendedExtension ])

And then run, we can see that addOptions is invoked twice--once where this.parent is undefined, and again when this.parent is [Function: bound addOptions]


So if i invove this.parent(), ExistingExtension throws.

const ExistingExtension = Node.create({
 addOptions(){
   console.log('existing', this.parent) // logs null
   return {}
 }
})

const ExtendedExtension = ExistingExtension.extend({
 addOptions(){
   console.log('extended', this.parent) // logs null
   return this.parent() // throws
 }
})

Codepen demo


So maybe this is a different bug entirely? This isn't a type issue after all but a runtime issue?

@nperez0111
Copy link
Contributor

Well look at that, addOptions is indeed incorrect here. What I said would be true for other things though like addAttributes, options are just treated differently and initialized immediately.

I was able to get options work with:

diff --git a/demos/src/Marks/Link/React/index.jsx b/demos/src/Marks/Link/React/index.jsx
index 4af6bcff6..43c45d6b3 100644
--- a/demos/src/Marks/Link/React/index.jsx
+++ b/demos/src/Marks/Link/React/index.jsx
@@ -16,65 +16,14 @@ export default () => {
       Paragraph,
       Text,
       Code,
-      Link.configure({
-        openOnClick: false,
-        autolink: true,
-        defaultProtocol: 'https',
-        protocols: ['http', 'https'],
-        isAllowedUri: (url, ctx) => {
-          try {
-            // construct URL
-            const parsedUrl = url.includes(':') ? new URL(url) : new URL(`${ctx.defaultProtocol}://${url}`)
+      Link.extend({
+        addOptions() {
+          console.log(this.parent)
+          return {
+            ...this.parent(),
 
-            // use default validation
-            if (!ctx.defaultValidate(parsedUrl.href)) {
-              return false
-            }
-
-            // disallowed protocols
-            const disallowedProtocols = ['ftp', 'file', 'mailto']
-            const protocol = parsedUrl.protocol.replace(':', '')
-
-            if (disallowedProtocols.includes(protocol)) {
-              return false
-            }
-
-            // only allow protocols specified in ctx.protocols
-            const allowedProtocols = ctx.protocols.map(p => (typeof p === 'string' ? p : p.scheme))
-
-            if (!allowedProtocols.includes(protocol)) {
-              return false
-            }
-
-            // disallowed domains
-            const disallowedDomains = ['example-phishing.com', 'malicious-site.net']
-            const domain = parsedUrl.hostname
-
-            if (disallowedDomains.includes(domain)) {
-              return false
-            }
-
-            // all checks have passed
-            return true
-          } catch (error) {
-            return false
-          }
-        },
-        shouldAutoLink: url => {
-          try {
-            // construct URL
-            const parsedUrl = url.includes(':') ? new URL(url) : new URL(`https://${url}`)
-
-            // only auto-link if the domain is not in the disallowed list
-            const disallowedDomains = ['example-no-autolink.com', 'another-no-autolink.com']
-            const domain = parsedUrl.hostname
-
-            return !disallowedDomains.includes(domain)
-          } catch (error) {
-            return false
           }
         },
-
       }),
     ],
     content: `
diff --git a/packages/core/src/Mark.ts b/packages/core/src/Mark.ts
index 6893bfce2..65c21afb3 100644
--- a/packages/core/src/Mark.ts
+++ b/packages/core/src/Mark.ts
@@ -550,7 +550,10 @@ export class Mark<Options = any, Storage = any> {
     defaultOptions: {},
   }
 
-  constructor(config: Partial<MarkConfig<Options, Storage>> = {}) {
+  constructor(config: Partial<MarkConfig<Options, Storage>> = {}, parent: Mark | undefined = undefined) {
+    if (parent) {
+      this.parent = parent
+    }
     this.config = {
       ...this.config,
       ...config,
@@ -608,7 +611,7 @@ export class Mark<Options = any, Storage = any> {
   extend<ExtendedOptions = Options, ExtendedStorage = Storage>(
     extendedConfig: Partial<MarkConfig<ExtendedOptions, ExtendedStorage>> = {},
   ) {
-    const extension = new Mark<ExtendedOptions, ExtendedStorage>(extendedConfig)
+    const extension = new Mark<ExtendedOptions, ExtendedStorage>(extendedConfig, this)
 
     extension.parent = this
 
diff --git a/packages/extension-link/src/link.ts b/packages/extension-link/src/link.ts
index b9b882995..e5ad5232d 100644
--- a/packages/extension-link/src/link.ts
+++ b/packages/extension-link/src/link.ts
@@ -237,6 +237,7 @@ export const Link = Mark.create<LinkOptions>({
   },
 
   addOptions() {
+    console.log('original options', this.parent)
     return {
       openOnClick: true,
       linkOnPaste: true,

To set the correct parent in the constructor. This would be a good thing to fix, looks like options were not being handled properly

@nperez0111 nperez0111 reopened this Nov 26, 2024
@mgreystone
Copy link
Contributor

So it seems the original issue i opened #5768 is not accurate & can be closed? The types are indeed correct & working as expected (for extend'd configs, at least).

I confess the documentation did confuse me, as all of the examples do use the this.parent?.() optional chaining, which sounds like it is not necessary? The documentation for attributes even explicitly says that optional chaining is required, though maybe that is still a special case for all i know.


Thanks you for everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug
Projects
None yet
Development

No branches or pull requests

3 participants