[gatsby-remark-copy-linked-files] Fix #2696 #2871

fk · 2017-11-10T03:13:52Z

…potentially without any side effects.

This removes a part of #1571 that replaces each Remark HTML node with its Cheerio .html()ified equivalent (and changes node.type to html). I'm not familiar at all with Cheerio (and have to stay ignorant for now due to time issues), but it seems that html() automagically closes open tags, e.g. an initial node.value of <a href="https://www.gatsbyjs.org"> becomes <a href="https://www.gatsbyjs.org"></a>.

Also, I only briefly looked into this and am not familiar at all with using the part of gatsby-remark-copy-linked-files that seems to have introduced the issue described in #2696.

From what I understand until now, the code I removed is not needed at all.
Maybe @chiedo can take a look again?

I also noticed that the same lines (including the comment) appear in

gatsby/packages/gatsby-remark-images/src/index.js

Lines 211 to 213 in ad0b4e8

    
           // Replace the image node with an inline HTML node. 
        
           node.type = `html` 
        
           node.value = $(`body`).html() // fix for cheerio v1

– maybe some copy pasta leftover?

gatsbybot · 2017-11-10T03:19:26Z

Deploy preview ready!

Built with commit 420c617

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

gatsbybot · 2017-11-10T03:21:15Z

Deploy preview ready!

Built with commit 420c617

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

fk · 2017-11-10T03:30:24Z

The PR currently modifies examples/using-remark to serve as a testing playground for this particular issue:

The "Hello World: The remark Kitchen Sink" article contains a "test" for [gatsby-transformer-remark] Content of inline HTML tags from markdown source is outside of tags in HTML output. #2696.
gatsby-remark-images is disabled in favor of the "just copy all supported images referenced in Markdown" functionality from Gatsby copy linked files, video tag support, a tag support and option to ignore default ignored file types #1571.

chiedo · 2017-11-10T12:14:42Z

Good work @fk. There have been a bunch of changes to gatsby-copy-linked files recently that broke my application so I'm back on my custom version of it... haha This looks good though

This reverts commit 9e75ca8.

This reverts commit b801bb6.

fk · 2017-11-10T15:59:21Z

@chiedo Thanks for taking a look!

There have been a bunch of changes to gatsby-copy-linked files recently that broke my application so I'm back on my custom version of it... haha

:-( Maybe file an issue? Looking at the gatsby-remark-copy-linked-files history there haven't been too many changes since your PR got merged.

This looks good though

I hope so! ;-)

I didn't manage to make room to really look closer today (let alone add tests), but at least I quickly tested BMP and TIFF files, rebuilt a ~150 Markdown files, ~100 images (JPG/PNG), ~30 linked and/or embedded PDFs site with and without gatsby-remark-images/gatsby-remark-copy-linked-files, and also skimmed through the relevant code again — I can't see why removing this should cause any problems/was added in the first place.

So I reverted my test commits in using-remark and will label this as good to go, fixes #2696 🎉

chiedo · 2017-11-10T16:12:30Z

Awesome @fk !

And if it's working for everyone else, i'll just keep using my custom version. It's been broken since my PR got merged (I just didn't realize until later). A lot of changes happened to merge it and then a few after.

But it's just a static site generator haha. So I'm not losing sleep about being on a fork.

KyleAMathews · 2017-11-10T18:56:27Z

@chiedo sorry to hear that :-( you mind adding a few failing tests with sample code from your site? That'd be super helpful even if you don't have time to chase down why things broke. Also ensure the plugin isn't broken again in the future :-)

KyleAMathews · 2017-11-10T18:56:42Z

Thanks @fk!!

chiedo · 2017-11-10T22:20:14Z

Hey @KyleAMathews,

Thanks for understanding the time predicament. haha I've been getting slammed for time each day... and haven't got to write code for like a month... All management and marketing these past few days.

I didn't want to create an issue because I don't have time to fix it and didn't want to be "that guy" haha

But this is the error I currently get when I just try to start my app with the new version of the plugin. I'm not sure what tests to include as sample code without including my whole codebase because I haven't been able to isolate the issue that's preventing my site from launching with gatsby develop when I use the plugin.

Error: Cannot find module '../build/Release/sharp.node'

  - module.js:11 require
    internal/module.js:11:18

  - constructor.js:8 Object.<anonymous>
    [marketing.chiedo.com]/[sharp]/lib/constructor.js:8:15

  - module.js:11 require
    internal/module.js:11:18

  - index.js:3 Object.<anonymous>
    [marketing.chiedo.com]/[sharp]/lib/index.js:3:15

KyleAMathews · 2017-11-10T22:29:13Z

That looks like an install error. Node is just saying it can't find a package. Try removing node_modules and reinstalling?

KyleAMathews · 2017-11-10T22:30:23Z

Also this plugin doesn't use sharp so the problems are unrelated.

P.s. sorry about no coding. Hopefully it means your rolling in the dough with huge projects!

chiedo · 2017-11-10T22:36:45Z

Haha thanks. That's the hope, a solid Q1, if all this marketing and management stuff pays off. Fingers crossed.

But yeah, I tried the removing of node_modules and reinstalling.

It's one of those freak things that is confusing me haha. If I remove that plugin, all is well. If I add the plugin, things go haywire.

At some point, things will slow back down again and I'll get to spend hours coding and debugging. Haha. But for now, my fork is a good temp solution.

ptrikutam · 2017-11-11T03:59:10Z

@fk this worked great for me. Thank you so much for the quick fix! You are awesome.

chiedo · 2017-11-11T13:05:31Z

Was able to do a little bit of Saturday debugging and was able to upgrade packages one-by-one :)

So... with all of that being said... An example of a blog post that works perfectly with my forked version but doesn't display any images with the current version can be downloaded here (The link is valid until Nov 18)

If someone wanted to debug, write tests, etc that would be awesome! It's something I'll eventually get to if not. But for now, using my forked version works for me.

fk · 2017-11-12T00:55:38Z

Bad news—just checked with regular HTML <img> tags (like the ones in @chiedos example Markdown), and those do not work anymore after my brilliant "fix" from this PR 🙄

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`…

…xp cannons (#2901) * Fix regression from #2871 with replaceString cannons Very icky, but works for now. I broke copying resources referenced for HTML-in-Markdown links, video sources, 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>. 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`… * Add examples/using-remark-copy-linked-files Not making real tests obsolete at all, but all I could come up with for now. * Tidy example * Drop replace-string, remove unused deps * Add test * Add test `can copy HTML file links` * Add test for `options.ignoreFileExtensions` * Add test `can copy HTML videos`

fk · 2017-11-13T20:30:48Z

@chiedo If you find the time please test your Markdown post(s) with the new [email protected]--your example post should work just fine now.✌️

chiedo · 2017-11-13T21:44:04Z

Works like a charm @fk way to go!!!

KyleAMathews · 2017-11-13T21:53:37Z

Yeah!

fk · 2017-11-13T22:14:23Z

🙏 @chiedo Glad to hear!

Florian Kissling added 3 commits November 10, 2017 03:44

Don’t replace the visited HTML node via Cheerio

1c56c16

Don’t use remark-images for examples/using-remark

9e75ca8

Add examples for gatsbyjs#2696 to the „Hello World“ article

b801bb6

Florian Kissling added 2 commits November 10, 2017 16:19

Revert "Don’t use remark-images for examples/using-remark"

9de3a67

This reverts commit 9e75ca8.

Revert "Add examples for gatsbyjs#2696 to the „Hello World“ article"

420c617

This reverts commit b801bb6.

KyleAMathews merged commit 4b444e3 into gatsbyjs:master Nov 10, 2017

fk mentioned this pull request Nov 13, 2017

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

Merged

fk mentioned this pull request Nov 13, 2017

Markdown images with space in filename break #2909

Closed

fk deleted the topics/fix-2696 branch November 14, 2017 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gatsby-remark-copy-linked-files] Fix #2696 #2871

[gatsby-remark-copy-linked-files] Fix #2696 #2871

fk commented Nov 10, 2017 •

edited

Loading

gatsbybot commented Nov 10, 2017 •

edited

Loading

gatsbybot commented Nov 10, 2017 •

edited

Loading

fk commented Nov 10, 2017 •

edited

Loading

chiedo commented Nov 10, 2017

fk commented Nov 10, 2017

chiedo commented Nov 10, 2017

KyleAMathews commented Nov 10, 2017

KyleAMathews commented Nov 10, 2017

chiedo commented Nov 10, 2017

KyleAMathews commented Nov 10, 2017

KyleAMathews commented Nov 10, 2017

chiedo commented Nov 10, 2017

ptrikutam commented Nov 11, 2017

chiedo commented Nov 11, 2017 •

edited

Loading

fk commented Nov 12, 2017

fk commented Nov 13, 2017

chiedo commented Nov 13, 2017

KyleAMathews commented Nov 13, 2017

fk commented Nov 13, 2017

	// Replace the image node with an inline HTML node.
	node.type = `html`
	node.value = $(`body`).html() // fix for cheerio v1

[gatsby-remark-copy-linked-files] Fix #2696 #2871

[gatsby-remark-copy-linked-files] Fix #2696 #2871

Conversation

fk commented Nov 10, 2017 • edited Loading

gatsbybot commented Nov 10, 2017 • edited Loading

gatsbybot commented Nov 10, 2017 • edited Loading

fk commented Nov 10, 2017 • edited Loading

chiedo commented Nov 10, 2017

fk commented Nov 10, 2017

chiedo commented Nov 10, 2017

KyleAMathews commented Nov 10, 2017

KyleAMathews commented Nov 10, 2017

chiedo commented Nov 10, 2017

KyleAMathews commented Nov 10, 2017

KyleAMathews commented Nov 10, 2017

chiedo commented Nov 10, 2017

ptrikutam commented Nov 11, 2017

chiedo commented Nov 11, 2017 • edited Loading

fk commented Nov 12, 2017

fk commented Nov 13, 2017

chiedo commented Nov 13, 2017

KyleAMathews commented Nov 13, 2017

fk commented Nov 13, 2017

fk commented Nov 10, 2017 •

edited

Loading

gatsbybot commented Nov 10, 2017 •

edited

Loading

gatsbybot commented Nov 10, 2017 •

edited

Loading

fk commented Nov 10, 2017 •

edited

Loading

chiedo commented Nov 11, 2017 •

edited

Loading