Feature: PMarkdownRenderer #637
Conversation
…ent without JSDOM and maybe not at all
|
There's 1 issue I'm still trying to address with this BUT it's not a blocker: table heights are infinite right now so I'm trying to find a solution that:
this is surprisingly more difficult than i anticipated, |
| const router = useRouter() | ||
|
|
||
| const kebabHash = computed(() => { | ||
| return kebabCase(props.hash ?? '') |
There was a problem hiding this comment.
This might be a bad contract but it does ensure that we have properly-formed urls absent encoded entities
|
seems like there's something up with the "live" demo |
|
Good catch @stackoverfloweth - looks like there's a piece of the table translation I hadn't accounted for that the fiber demo is using; working on that now! |
| import { kebabCase } from '@/utilities' | ||
|
|
||
| const props = defineProps<{ | ||
| depth?: number, |
There was a problem hiding this comment.
what does depth do?
There was a problem hiding this comment.
Depth determines the heading tag used for semantic markup (h1, h2, etc); makes it really easy to use the component as-is if you're styling headers on tags instead of classes without additional wrappers/classes
| import { marked } from 'marked' | ||
| import { computed, ref, watch } from 'vue' | ||
| import { getRootVNode } from '@/components/MarkdownRenderer/parser' | ||
| import MarkdownTokenWorker from '@/components/MarkdownRenderer/worker?worker&inline' |
There was a problem hiding this comment.
?worker?inline? is that intentional?
There was a problem hiding this comment.
You know I'm not sure how a chunked worker would behave in the package since we haven't done that before... so I guess yes it's intentional only because I've not tried it any other way
| @@ -0,0 +1,206 @@ | |||
| import { marked } from 'marked' | |||
There was a problem hiding this comment.
is it true that is only used in PMarkdownRenderer? Is there a reason to break it out into a separate ts file? Or was that only to keep the vue file shorter?
There was a problem hiding this comment.
Yeah just here! And yeah that's exactly right; ideally I'd break this file out even further into factory classes like you mentioned here
| export const getRootVNode = (tokens: marked.TokensList | [], options: ParserOptions): VNode => { | ||
| const children: VNode[] = tokens.map((token) => getVNode(token, options)) | ||
| return h('article', { class: [baseClass] }, children) | ||
| } |
There was a problem hiding this comment.
I think this file should probably be arranged slightly different in the future. Perhaps more of a factory pattern where each supported component gets it's own file/class for parsing token. But for a 1st pass I think this is easy enough to follow 👍
There was a problem hiding this comment.
Agreed! I had the same thought last night that I'd break these out if I realized how many different branches this would take. If anyone else is finding this tough to follow and would prefer I took the time to break them out now I'm game, otherwise will focus attention elsewhere.
|
All feedback has been addressed 👍🏻 |
| @@ -24,14 +24,13 @@ | |||
| tokens.value = message.tokens | |||
| } | |||
|
|
|||
| const worker: Worker = new MarkdownTokenWorker() | |||
| const worker = new MarkdownTokenWorker() | |||
There was a problem hiding this comment.
eslint says something is unused now
There was a problem hiding this comment.
do you have eslint running in your vscode?
There was a problem hiding this comment.
Ah yea I missed an issue there cause I'm getting spammed with unknown css rules for tailwind despite us having definitions for those @rules
src/types/markdownRenderer.ts
Outdated
| return 'tokens' in token | ||
| } | ||
|
|
||
| export function isCodeBlock(token: Token): token is Token & { type: 'code' } { |
There was a problem hiding this comment.
all of these types guard below aren't doing anything that an inline if wouldn't equally accomplish. Usually a type guard checks a couple of things and changes the input from unknown to something specific. These are taking Token and returning Token but with a falsy check basically already done. Unless these are really reused in a lot of places, I would just do your falsy check in the component
There was a problem hiding this comment.
I've tightened the input restriction to unknown but I don't think you're correct that a type guard has to take an unknown type, even in this codebase, and certainly not more broadly. The point of these is to sufficiently narrow a Token to a more actionable subtype (I'm not sure I understand what you mean by returning a token with a falsy check).
An inline if statement would do the same thing, you're right, but that's what was here when you asked for a type guard, so maybe I misunderstood your initial comment. Keeping these guards inline is less readable (in my opinion) and more repetitive.
There was a problem hiding this comment.
Sorry for being misleading. I do not believe every type guard needs to take unknown, the point I was trying to make (poorly) is that a type guard IMO should take some type and through some condition return a different type. I think what I was hoping for in my original comment about type guards was that there was a different sub type that we could use in a function like mapChildTokens. In that case, a type guard would be a great way to narrow from Token to ParentToken or whatever and then feed it to the properly function. In this case, the type you're returning is the same type you were given, but with a single truthy check performed on it. Maybe that's fine? Just seemed strange to me. Feel free to disregard
There was a problem hiding this comment.
Oh! In which case I think you'll be happy with the updated type guards - they're functionally the same but they do return a different type in the marked.Tokens namespace
|
|
||
| try { | ||
| const url = new URL(route) | ||
| return url.host !== window.location.host |
This PR introduces a markdown-rendering component which wraps the
markedanddompurifylibraries to deliver prefect-design-enabled components built off markdown + html strings.The lexer (the program that extracts tokens from the string) is run in a worker similar to the one used by the
PCodeHighlightcomponent but sanitization (removing any potentially dangerous HTML attributes or tags) happens in the main thread. This is because it's not possible to usedompurify(or its wrapped siblingisomorphic-dompurify) without at least introducing JSDOM and even then it may not work.Markdown support to add:
jsis added tojavascript, for example)