laggingreflex/preact-markdown

Consider marked instead of markdown

Closed this issue · 9 comments

simevidas commented

I’ve done a little bit of testing (on the official preact-boilerplate project) and it looks like switching from markdown to marked leads to smaller bundle sizes, specifically the bundle size increases 27.8 KB with preact-markdown, but only 21 KB with the marked-based approach (in the most basic scenario).

Before:

before

After rendering a simple <h2> via preact-markdown:

preact-markdown

After rendering the same <h2> via the markdown.js component used on Preact’s website (which uses marked):

preact-www-markdown

laggingreflex commented

Sorry for the late reply. I did some comparisons and also found size differences: #2

But it seems to mess up styles in some places, for eg.:
image

Not sure if it's due to lack of setting the right options or something else interfering. I've merged the PR nonetheless (0.2.0 released). setOptions could be supported in the future.

simevidas commented

I’ve hit a different issue:

My Markdown code includes a few images using the ![alt text](image URL) syntax. After updating to version 0.2.0, preact-markup now throws an error about an “opening and ending tag mismatch” related to the image. So, I guess it expects a self-closed image tag (<img/> instead of <img>), and marked generates the latter.

I’m not sure where this issue should be reported.

That being said, the Preact webiste (preact-www repo) which uses marked in combination with preact-markup, also uses images in its Markdown code (example) and it seems to get away with it. I’ll investigate.

  • 👍1
simevidas commented

I think I’ve fixed it. I’ve added type: 'html' in your code (see // ADDED comments) and my pages with images now render. This change may also fix your issues, so try it out :)

function Markdown(props) {
  if (typeof props === 'string') {
    return h(Markup, {
      markup: marked(props),
      type: 'html', // ADDED
      trim: false,
    })
  } else if (props && props.markdown) {
    return h(Markup, Object.assign({
      markup: marked(props.markdown),
      type: 'html', // ADDED
      trim: false,
    }, props))
  } else {
    throw new Error('Invalid arguments. Markdown requires either a `<String>` or object: `{markdown: <String>}`')
  }
}

Btw, my own bundle.js size went down from 63.5 KB to 55.5 KB 🙌

  • 👍1
laggingreflex commented

patched 3702007 released 0.2.1

  • 👍2
laggingreflex commented

type: 'html' wraps the body in '<!DOCTYPE html><html><body>' + markup + '</body></html>'. This started causing an issue for me but strangely it only happens when I have this plugin:

new webpack.LoaderOptionsPlugin({ minimize: true })

(with that minimize option set to true) in my webpack config (which I was only using in prod)

@simevidas Were you using this plugin by any chance?

In any case, I'm thinking it might be better to let the user specify both preact-markup and marked options themselves. Does #3 seem good?

simevidas commented

I’ve looked at the source code of <Markup> and Preact’s website. A few things:

  1. If type="html" is not set, preact-markup will parse the markup as XML. Unless there’s a use case I’m missing, I think we never want to parse as XML, due to the limitations it imposes (see 3.).

  2. Preact’s website uses <Markup> twice—here and here—and in both cases type="html" is set.

  3. Leaving out type="html" breaks when the Markdown code contains images, as we’ve already discovered, and now we know why: Marked generates <img> tags which, when parsed as XML, throw an error (because they’re not self-closed). The same issue is true for other types of empty HTML elements that appear in the Markdown code, either directly like <input>, or as Markdown, like *** which Marked converts to <br>.

Based on that information, I think it makes most sense to default to type="html". Regarding cases where this causes an issue, I see two possible approaches:

  1. <Markup> could be changed so that it doesn’t wrap in <!DOCTYPE html> and instead uses a different way to parse the HTML.
  2. <Markdown> could provide something like parser="xml" which would set type="xml" on <Markup> to use XML instead of HTML parsing. But since this prevents empty elements from being used in the Markdown source, it would be a last resort with limited functionality.

I don’t use LoaderOptionsPlugin (I’ve started using Webpack with version 2), but it might be worth investigating if the wrappedMarkup line in <Markup> could be changed to something else, so that it doesn’t cause the issue. (You can just edit the file directly in /node_modules/preact-markup/parse-markup.js for some quick and dirty testing. Ah, you can’t. It runs from the generated script in the /dist directory.)

laggingreflex commented

I've kept html as default. With the ability to specify options it can still be overridden with {markupOpts: {type: xml}}. For my case I've actually stopped using that loader plugin, not sure why I was still using it, I'm on Webpack 2 as well.

laggingreflex commented

(You can just edit the file directly in /node_modules/preact-markup/parse-markup.js for some quick and dirty testing. Ah, you can’t. It runs from the generated script in the /dist directory.)

Btw, I too struggle with this a lot while debugging modules. I usually fork/clone the said module in ~/forks/preact dir and then use npm link preact in the current project. You can have the forked module's build command run in parallel to auto update with its dist folder. I've written gfork to make this easier.

developit commented

This is great stuff - I think we can switch preact-www over to using this module.