remarkjs / remark-react

Legacy plugin to transform to React — please use `remark-rehype` and `rehype-react` instead
https://unifiedjs.com
MIT License
524 stars 37 forks source link

Externalise core #21

Closed wooorm closed 8 years ago

wooorm commented 8 years ago

Hi folks 👋

I did quite some refactoring / added some features. I split the changes into two commits; that way it’s easier to differentiate between what’s what. The first commit (update dependencies, dev-dependencies) deals with all kinds of summer-cleaning. The second externalises a lot of the core from remark-react into another package. That packages, mdast-util-to-hast, is also used by remark-html and remark-vdom (the latter very soon).

The first ensures some of the stuff from way back to mdast-html was cleaned out; I added Mapbox copyright, and some contributors; plus I removed a lot of unused stuff, such as docs for non-existant settings and unused configuration files. It’s pretty big, sorry!

The last commit comes with three caveats: a) no spans are added around inline text; b) white-space newlines are added between nodes*; c) setting a component for inlineCode is no longer supported** Plus cool features: It’s now possible to specify whether ids get sanitised by passing a sanitation schema; and it’s possible to integrate with other remark tools like remark-highlight.js (very soon)

Lot’s of changes, sorry about that. Please let me know about any Q’s, I’d love to expand where need be!

P.S. I also tested on React@15.0 and everything went OK! 👌 Didn’t include it in these commits though.

* can be removed if you’d rather not have it in; ** If this is very important I’d like to figure out a way to get it back in!

wooorm commented 8 years ago

P.P.S. This should close GH-19 and GH-14. And since GH-5 is already fixed, and GH-11 can be done by passing in a component, this should bring all issues / PRs down to 0!

tmcw commented 8 years ago

Awesome! I'm about to head out for a vacation - @scothis want to take a look at this excellent PR and merge if it all looks 👍 ?

wooorm commented 8 years ago

Cool! Enjoy vacation, Tom!

@scothis Hi ☺️ Do let me know if I can elaborate on this PR!

scothis commented 8 years ago

@wooorm sorry for the delay. I have to plead ignorance when it comes to the remark code base, as such I'm not really in a good position to judge these changes. @tmcw do you want to pick this one back up? Otherwise I can dig in deeper.

tmcw commented 8 years ago

👍 awesome, did a review. hast-util-sanitize covers all the bases we did previously. I'm a little confused by some of the behavior: for instance, the html-sanitize fixture:

Removes the h1 title entirely.

wooorm commented 8 years ago

@tmcw Struck me as weird, too, but I believe I can clarify!

So, <h1> is a block of HTML, and blocks of HTML are completely stripped (as of now). The rest is inline HTML, and that is smartly parsed and stripped of tags. A little parsing of HTML goes a long way in markdown, it allows you to mix markdown with HTML if you trust the source (<sup>*foo*</sup>). Although <h1>*foo*</h1> looks similar, it’s treated differently by most markdown implementations.

The current master branch displays the HTML (encoded, so &lt;h1>). Would you rather have that behaviour?

I believe the stripping behaviour makes more sense for sanitation (I’ve included a <script>alert(1)</script> but it’s completely removed by GH here: ).

But then again simply displaying HTML which cannot be converted might be better for markdown to virtual dom.

P.S. Thanks for reviewing! 👍

tmcw commented 8 years ago

👍 makes sense, and since the current master branch has... well, less optimal, behavior I'm convinced we should switch. Merging.

wooorm commented 8 years ago

Coool! 👍

As an aside, I’m investigating whether I could do something like real HTML (in markdown) to virtual DOM / React conversion by mixing rehype and remark. I’m not exactly sure about how to do it though, and it won’t be very small of course (including two parsers), but it would be cool nonetheless :)

wooorm commented 8 years ago

@tmcw Oh and I believe this also closes the other open issues/PRs (see https://github.com/mapbox/remark-react/pull/21#issuecomment-228074615).

tmcw commented 8 years ago

Ah, working through upgrading the example and I think this unfortunately regresses the admittedly hacky system for keys that we had before. It works, but React warns. You can see the warning by opening https://mapbox.github.io/remark-react/

wooorm commented 8 years ago

Ah darn, Sorry I didn’t check that. brb, reading the docs :)

Maybe passing in a key to toH() would work?

wooorm commented 8 years ago

Think I found it it in hast-to-hyperscript, which doesn’t properly detect React 14 for some reason, I’ll have a patch soon!

wooorm commented 8 years ago

OK, I tested locally / npm linked and the upcoming patch fixes this. It’s automatically released as 2.0.3 by Travis if this build finishes: https://travis-ci.org/wooorm/hast-to-hyperscript/jobs/144011542.

wooorm commented 8 years ago

@tmcw Landed & released!

tmcw commented 8 years ago

👍 excellent, rebuilt the gh-pages with the change. Thanks!