Migrate core/src/lib/markdown-it to TypeScript #2231
Conversation
jovyntls
left a comment
jovyntls
left a comment
There was a problem hiding this comment.
Thanks for fixing the weird git issues @yucheng11122017 :)
Left a few comments!
| markdownIt.renderer.rules.fence | ||
| = (tokens: Token[], idx: number, options: Options, env: any, slf: Renderer) => { |
There was a problem hiding this comment.
| markdownIt.renderer.rules.fence | |
| = (tokens: Token[], idx: number, options: Options, env: any, slf: Renderer) => { | |
| markdownIt.renderer.rules.fence = (tokens: Token[], | |
| idx: number, options: Options, env: any, slf: Renderer) => { |
This indentation can help avoid unnecessary whitespace changes throughout the function, which will help git recognise index.ts as a refactor instead of a new file.
|
|
||
| return new HighlightRule(components); | ||
| const componentsWithoutNull: HighlightRuleComponent[] = []; | ||
| components.forEach((c: HighlightRuleComponent | null) => { |
There was a problem hiding this comment.
is it possible to use filter(component => component !== null) here instead?
There was a problem hiding this comment.
No sadly.. the type is still (HighlightRuleComponent | null) [] after filtering
By right, there shouldn't be any null anyway because of the previous check some(c => !c) but this is not recognised by the linter
There was a problem hiding this comment.
Ah I see, thanks for the clarification 😭 In that case could we do a typecast directly in the return (component as HighlightRuleComponent[]) since we're sure that there shouldn't be nulls?
| const lineNumberStr = groups.shift(); | ||
| if (lineNumberStr === undefined) { | ||
| return null; | ||
| } | ||
| let lineNumber = parseInt(lineNumberStr, 10); |
There was a problem hiding this comment.
| const lineNumberStr = groups.shift(); | |
| if (lineNumberStr === undefined) { | |
| return null; | |
| } | |
| let lineNumber = parseInt(lineNumberStr, 10); | |
| let lineNumber = parseInt(groups.shift() ?? '', 10); |
Since parseInt('') will be NaN!
There was a problem hiding this comment.
This file is currently ignored by .eslintignore - can we add it back to ensure that it's linted?
| const key: IconName | undefined = (Object.keys(octicons) as Array<IconName>).find(key => key === iconName); | ||
| if (key === undefined) { | ||
| return null; | ||
| } | ||
| return octicons[key]; |
There was a problem hiding this comment.
| const key: IconName | undefined = (Object.keys(octicons) as Array<IconName>).find(key => key === iconName); | |
| if (key === undefined) { | |
| return null; | |
| } | |
| return octicons[key]; | |
| return octicons[iconName as IconName]; |
or if we need to return null instead of undefined:
| const key: IconName | undefined = (Object.keys(octicons) as Array<IconName>).find(key => key === iconName); | |
| if (key === undefined) { | |
| return null; | |
| } | |
| return octicons[key]; | |
| return octicons[iconName as IconName] ?? null; |
| const octiconIconHtml = iconClass | ||
| ? octiconIcon.toSVG({ class: iconClass }) | ||
| : octiconIcon.toSVG(); | ||
| const $ = cheerio.load(octiconIconHtml); | ||
| $('svg').attr('style', 'color: #fff'); | ||
| return $.html(); |
There was a problem hiding this comment.
Is there a reason why we are performing this transformation in cheerio instead of passing style as an option in the toSVG method?
There was a problem hiding this comment.
the interface of SVG options doesn't have the attribute style😭
There was a problem hiding this comment.
Ah ic, thanks for the workaround!
yucheng11122017
force-pushed
the
migrateMarkdownItTs
branch
from
730afac to
8828821
Compare
|
Thanks for the comments @jovyntls! I updated accordingly |
jovyntls
left a comment
jovyntls
left a comment
There was a problem hiding this comment.
LGTM! lib/markdown-it in TS is nice to have 🎊
|
@yucheng11122017 Feel free to try merging this (rebase strategy) once it's ready - after a second approval if possible! |
3 participants
What is the purpose of this pull request?
Part of #1913.
Duplicate of #2226
Overview of changes:
Migrates core/src/lib/markdown-it to TypeScript
Per #1913, the other files in this folder won't be migrated to TS for now because they are mostly from other source codes.
Anything you'd like to highlight/discuss:
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Rebase commit
Checklist: ☑️