Skip to content
This repository has been archived by the owner on Feb 28, 2022. It is now read-only.

Fix problems in the markdown -> html pipeline #163

Merged
merged 5 commits into from Jan 26, 2019
Merged

Conversation

koraa
Copy link
Contributor

@koraa koraa commented Jan 18, 2019

This fixes two issues with markdown rendering and adds an infrastructure for testing whether for a specific markdown input, the correct output is produced.

Not sure whether test/testHTMLFromMarkdown.js should not be merged into test/testHTML.js; I kind of like having all the markdown tests in their own place, on the other hand a lot of boilerplate is required to put them there…

@koraa
Copy link
Contributor Author

koraa commented Jan 18, 2019

I am also pondering what would be a good place for a domEquals function? Thinking of extracting one from assertDomEquals and putting those two functions somewhere where all helix code can use it…

@trieloff
Copy link
Contributor

We may want to create a new helix-testutils project. I need to put my condit method for conditional integration testing somewhere, too.

@koraa koraa force-pushed the karo/markdown-fixes branch 2 times, most recently from 7c1c313 to 1eb1cb1 Compare January 18, 2019 15:18
@koraa
Copy link
Contributor Author

koraa commented Jan 18, 2019

After fixing the missing support for the missing "referenceType" attribute in the mdast json schema, I can't get the example below to work; needs extra investigation but I do not think this is any error we introduced, since this is a highly specific property of the GFM & CommonMark specs (which flavor are we using by the way).
Pondering whether this is worth fixing if it's somewhere deep inside mdast…in general IMHO it's worth fixing those really finicky errors because they make for even weirder edgecases and issues…

As far as I can see, the markdown is already being parsed incorrectly…it parses [link] as a reference and (<foobar) as adjacent text…

Input Markdown

# Foo

Hello World [link](<foobar)

Expected result

(As rendered by marked with GFM enabled and without)

<h1 id="foo">Foo</h1>
<p>Hello World <a href="%3Cfoobar">link</a></p>

Actual Result

<h1>Foo</h1>
<p>Hello World [link](&lt;foobar)</p>

MDAST

{ type: 'root',
  children:
   [ { type: 'heading',
       depth: 1,
       children:
        [ { type: 'text',
            value: 'Foo',
            position:
             Position {
               start: { line: 1, column: 3, offset: 2 },
               end: { line: 1, column: 6, offset: 5 },
               indent: [] } } ],
       position:
        Position {
          start: { line: 1, column: 1, offset: 0 },
          end: { line: 1, column: 6, offset: 5 },
          indent: [] } },
     { type: 'paragraph',
       children:
        [ { type: 'text',
            value: 'Hello World ',
            position:
             Position {
               start: { line: 3, column: 1, offset: 7 },
               end: { line: 3, column: 13, offset: 19 },
               indent: [] } },
          { type: 'linkReference',
            identifier: 'link',
            label: 'link',
            referenceType: 'shortcut',
            children:
             [ { type: 'text',
                 value: 'link',
                 position:
                  Position {
                    start: { line: 3, column: 14, offset: 20 },
                    end: { line: 3, column: 18, offset: 24 },
                    indent: [] } } ],
            position:
             Position {
               start: { line: 3, column: 13, offset: 19 },
               end: { line: 3, column: 19, offset: 25 },
               indent: [] } },
          { type: 'text',
            value: '(<foobar)',
            position:
             Position {
               start: { line: 3, column: 19, offset: 25 },
               end: { line: 3, column: 28, offset: 34 },
               indent: [] } } ],
       position:
        Position {
          start: { line: 3, column: 1, offset: 7 },
          end: { line: 3, column: 28, offset: 34 },
          indent: [] } } ],
  position:
   { start: { line: 1, column: 1, offset: 0 },
     end: { line: 3, column: 28, offset: 34 } } }

HAST

{ type: 'root',
  children:
   [ { type: 'element',
       tagName: 'h1',
       properties: {},
       children:
        [ { type: 'text',
            value: 'Foo',
            position:
             { start: { line: 1, column: 3, offset: 2 },
               end: { line: 1, column: 6, offset: 5 } } } ],
       position:
        { start: { line: 1, column: 1, offset: 0 },
          end: { line: 1, column: 6, offset: 5 } } },
     { type: 'text', value: '\n' },
     { type: 'element',
       tagName: 'p',
       properties: {},
       children:
        [ { type: 'text',
            value: 'Hello World ',
            position:
             { start: { line: 3, column: 1, offset: 7 },
               end: { line: 3, column: 13, offset: 19 } } },
          { type: 'text',
            value: '[link]',
            position:
             { start: { line: 3, column: 14, offset: 20 },
               end: { line: 3, column: 18, offset: 24 } } },
          { type: 'text',
            value: '(<foobar)',
            position:
             { start: { line: 3, column: 19, offset: 25 },
               end: { line: 3, column: 28, offset: 34 } } } ],
       position:
        { start: { line: 3, column: 1, offset: 7 },
          end: { line: 3, column: 28, offset: 34 } } } ],
  position:
   { start: { line: 1, column: 1, offset: 0 },
     end: { line: 3, column: 28, offset: 34 } } }

@koraa koraa force-pushed the karo/markdown-fixes branch 6 times, most recently from 0da7757 to 0fea673 Compare January 18, 2019 16:45
@codecov
Copy link

codecov bot commented Jan 18, 2019

Codecov Report

Merging #163 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #163   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          39     39           
  Lines         839    833    -6     
  Branches      164    160    -4     
=====================================
- Hits          839    833    -6
Impacted Files Coverage Δ
src/html/emit-html.js 100% <100%> (ø) ⬆️
src/defaults/html.pipe.js 100% <100%> (ø) ⬆️
src/utils/mdast-to-vdom.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed30562...a4b33c6. Read the comment docs.

@koraa
Copy link
Contributor Author

koraa commented Jan 18, 2019

Please do not merge yet; wip

@koraa
Copy link
Contributor Author

koraa commented Jan 18, 2019

Btw, this PR removes the VDOM.process function; is it used anywhere outside pipe?

@trieloff trieloff changed the title Fix problems in the markdown -> html pipeline WIP: Fix problems in the markdown -> html pipeline Jan 21, 2019
Copy link
Contributor

@trieloff trieloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using GFM. If you've discovered an issue in the remark tools, file an issue against the project and create a PR. @woorm is generally fast to respond, review, merge and release.

* Turns the MDAST into a basic DOM-like structure using Hyperscript
* @returns {Node} a simple DOM node (not all DOM functions exposed)
*/
process() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of this function is to be used in project-code, e.g. if you want to override the way HTML is being generated, you'd be able to register a new handler and then run process in your pre.js

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I guess just using getDocument is easier.

@koraa koraa changed the title WIP: Fix problems in the markdown -> html pipeline Fix problems in the markdown -> html pipeline Jan 25, 2019
Content can be directly provided to the html pipeline;
in this mode the pipline does not make an http request
to fetch the data, instead the content provided is used.

Until this commit, the pipeline would disregard the content
passed to it, if the content is an empty string.
In this case an http request would be performed to fetch
the data.

This behaviour is fixed by this commit
This wrapping div would be added when there was more then one
element in the markdown (which is the usual case) by hyperscript
because hyperscript has no concept of multiple elements in the
root node.

Our code came to expect this wrapping which led to markdowns with
a single element being rendered as an empty document in some
cases (the single element would be deleted during the emit-html
when generating .content.children).

This commit fixes this issue by using hast-util-to-html instead of
hyperscript for the hast -> html conversion which does indeed have
a concept of multiple elements at the root node.

Fixes #157
@koraa
Copy link
Contributor Author

koraa commented Jan 25, 2019

Should be ready to merge as soon as the branch in helix-shared is merged.
Please merge in separate commits (already squashed into sensible atomic changes)

@tripodsan tripodsan merged commit bf21862 into master Jan 26, 2019
@kptdobe
Copy link
Contributor

kptdobe commented Jan 26, 2019

arff... no... too fast...
I use the process method because it returns a node only and not a full document! See https://github.com/kptdobe/helix-sections-playground/blob/master/src/html.pre.js#L50

@trieloff trieloff mentioned this pull request Jan 28, 2019
@koraa koraa deleted the karo/markdown-fixes branch February 5, 2019 16:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants