Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gatsby-remark-copy-linked-files] Fix regression from #2871 with RegExp cannons #2901

Merged
merged 9 commits into from
Nov 13, 2017

Conversation

fk
Copy link
Contributor

@fk fk commented Nov 13, 2017

Sooooo…I broke resource references being updated for HTML-in-Markdown links, videos, and images in #2871 when trying to fix #2696 by removing writing back the HTML node.value via Cheerio’s .html(). 🙈

Cheerio closes open HTML tags, which in our case caused the problems described in #2696—an HTML node a with value <a href=…> would be written back as <a href=…></a>, putting the link children after the link:

const $ = cheerio.load('<a href="http://example.com/">')
$.html()
//=> <a href="http://example.com/"></a>

image

Not serializing the Cheerio object of our node back to node.value of course resulted in the resource attributes altered with it being lost.


Since I couldn’t figure out how to use Cheerio nor Rehype/unified to a) comfortably and relatively safely update the src and href attributes while b) returning just a fragment of the HTML node (<a href=...> instead of <a href=...></a>) , here’s an ugly beginner fix 😬 that replaces the resource links directly in node.value

All in all this is merely trying to follow up on the mess I made in #2871 as good as my available time currently allows – not convinced about this approach at all, so I left the replace stuff as dumb as possible for now and added a new example instead: examples/using-remark-copy-linked-files that hopefully makes it a little easier for other people to take a look. ⛑ 🙏

Here's the new example site's main Markdown file, exploring the AST might help understanding my blatherings: https://astexplorer.net/#/gist/307f2f3226954fa34304503a8d9c6cde/c52af3223c9e2b3a14f21ec0df00f143cd688037

Florian Kissling added 2 commits November 13, 2017 04:53
Very icky, but works for now.

I broke copying resources referenced for HTML-in-Markdown links, video sources, and images in gatsbyjs#2871 when trying to fix gatsbyjs#2696 by removing writing back the HTML `node.value` via Cheerio’s .html(). Cheerio closes open HTML tags, which in our case caused the problems described in gatsbyjs#2696—an HTML node a with value `<a href=…>` would be written back as `<a href=…`></a>.

Since I couldn’t figure out how to do this with either Cheerio nor Rehype/unified, here’s a beginner fix 😬 that replaces the resource links directly in `node.value`…
Not making real tests obsolete at all, but all I could come up with for now.
@gatsbybot
Copy link
Collaborator

gatsbybot commented Nov 13, 2017

Deploy preview ready!

Built with commit 78f43d7

https://deploy-preview-2901--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Nov 13, 2017

Deploy preview ready!

Built with commit 78f43d7

https://deploy-preview-2901--using-drupal.netlify.com

@fk fk changed the title [gatsby-remark-copy-linked-files] Fix regression from #2871 with replaceString cannons [gatsby-remark-copy-linked-files] Fix regression from #2871 with RegExp cannons Nov 13, 2017
@KyleAMathews
Copy link
Contributor

This is great! Cheerio is nice but yeah, didn't turn out to be a great fit for us as they want to create "well-formed" HTML which doesn't always jive with user input or perhaps how Remark parses things. So yeah, Regex seems like the way to go to ensure we're not breaking people's HTML.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants