Added optional custom uri validator in link extension #5726
Conversation
|
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| * @param url - The url to be validated. | ||
| * @returns - True if the url is valid, false otherwise. | ||
| */ | ||
| customUriValidator?: (url: string | undefined) => boolean |
There was a problem hiding this comment.
I like this, but now we have two validation options:
validatecustomUriValidator
This could be confusing, especially since they don't apply to the same places. Maybe we should merge the implementation into a single validation function that is used everywhere?
There was a problem hiding this comment.
I see what you mean, and I have thought about it. However, there must be a reason why the validate function was not used when this change was made: https://github.com/ueberdosis/tiptap/pull/5470/files#diff-1d1f108bd30f776ec9689dc1bb00a3cb3dafc89fde595994aa8fa400471a87d6. Therefore, I have decided to keep these functionalities separate.
There was a problem hiding this comment.
@nperez0111 What are your thoughts here? Also, would it be possible to have this one fully reviewed soon? 🙏
There was a problem hiding this comment.
I, too, would not make them be separate functions since they are too similar in name.
I can't speak to why this what not done like this here: https://github.com/ueberdosis/tiptap/pull/5470/files#diff-1d1f108bd30f776ec9689dc1bb00a3cb3dafc89fde595994aa8fa400471a87d6 but, I think that if a custom validator is passed, it is expected to be used properly.
Ultimately, I'd have:
validate: (url: string, defaultValidate: (url: string) => boolean ) => booleanThis would allow the user to specify their own validation, and if they don't want to both writing their own rules, they can use our existing defaultValidate function (which currently maps to isAllowedUri with the protocols being prefilled.
There was a problem hiding this comment.
Yes, they are similar in name, but they serve different purposes. The validate function is solely used for the autolink option, while the newly added one ensures that there are no malicious URLs. If these functionalities were handled by a single function, it would be impossible to differentiate between them, meaning all valid links would be autolinked. I don't think that's something you would want. For example, should relative links be autolinked? In my use case, this is something I do not want.
There was a problem hiding this comment.
I see your point with auto linking but I feel that validation should be about validity. How about we add a new callback for shouldAutoLink where you could reject a relative link separately from validation?
There was a problem hiding this comment.
Please correct me if I'm mistaken, but it sounds like you're suggesting a simple rename from validate: (url: string) => boolean to shouldAutoLink(validate: (url: string) => boolean), right? From what I understand, the validate method is an optional/custom method meant to override the default behavior of the autolink: boolean.
The main issue seems to be that the method’s name doesn't clearly reflect its purpose —the validate function doesn’t actually validate links, but rather checks whether a link should be auto-linked.
The customUriValidator method could be renamed to validate, but personally, I’d keep the name customUriValidator since it more clearly describes the function’s role."
There was a problem hiding this comment.
To make my case, take this example:
Link.configure({
validate: () => true,
customUriValidator: () => true
})This does not "more clearly describes the function’s role", it makes it confusing to know which applies to auto-linking and which applies to actual validation.
The current validate function, as I understand, is used in two cases, for auto-linking and matching against pasted text when looking for links. But, we are introducing a 3rd case where it will be useful for determining what we consider to be a "valid" URL (i.e. not malicious).
You wanted to support the ability to differentiate what would be considered for auto-linking from what is considered a valid link. I offered you a way to do this, a new function which would be specifically for auto-linking behavior to choose when something would be auto-linked (assuming it was already valid).
So, this is what I'm proposing:
Link.configure({
validate: (url: string, defaultValidate: (url: string) => boolean) => boolean
shouldAutoLink: (url: string) => boolean
})If a link is not valid (according to the validate function), then it is not considered as a link that can be linked to and will be dropped.
If a link is considered valid (according to the validate function), then the shouldAutoLink function will be called to allow rejecting links that we may consider valid, but do not want to auto-link to.
Does this sound reasonable?
There was a problem hiding this comment.
I realize I wasn’t entirely clear in my PR. For me, the most important thing is that TipTap can handle relative links again. I initially tried to fix this using the validate function, but I found that it was only used for autolinking. That's why I added the additional customUriValidator. I forgot to add why I was adding this.
In my previous comment, what I was trying to propose was:
Link.configure({
validate:
shouldAutoLink:
})
For now, I’m going to enjoy my vacation, and I’ll get back to this afterward.
|
This #5808 pull request appears to contain the suggested change, so I’m closing this one. |
Changes Overview
Added custom url validation to link extension
Implementation Approach
Added an optional custom url validation to provide full control over (in)valid uri's
Testing Done
Added test
Verification Steps
Additional Notes
Checklist
Related Issues
[Bug]: Expose an API to overwrite the default link validation