feat(gatsby-remark-embed-snippet): apply gatsby-remark-prismjs configuration to embedded snippets

[timhagn]

Description

This is the continuation of PR #13841, which I sadly botched % ).
Added my changes here again on a clean fork.

While writing a documentation page for gbi, I noticed that line numbering and some other options of gatsby-remark-prismjs weren't working here. Stepping through it with Intellij Idea's Debugger confirmed that the visit(markdownAST, code, node => {...} branch of it was of course never reached, as this plugin already had processed it to HTML.
So I changed the constructed node.type to code, just gave the read in file as node.value and added node.lang to the returned node, which are needed for further processing down the line.
It just seemed unnecessary to duplicate gatsby-remark-prismjs's code-highlighting functionality by creating nearly the same output (and it should prevent any future deviations).

Related Issues

Fixes the issue I would have opened otherwise ; ).

[pieh]

It looks good 👍

I think this change warrant doing major version bump for gatsby-remark-embed-snippet as there is potential for changed behaviour when using it with same configuration (i.e added line numbering).

[timhagn]

Hi @pieh, thanks for the thumbs up : )!

Though as I already tested it in reactjs.org (see this comment from the first PR #13841) and it doesn't seem to break anything in combination with the current gatsby-remark-prismjs - and -embed-snippets now just follows its default node creation / transformation process - I don't think a major version bump would really be necessary.
But that's up to you people, of course ; ).

[pieh]

What I meant by changed behaviour, is that if you specify line numbering option (right now), it won't be used for embedded snippets. If I understand changes here correctly - this will fix it, so fixing this, also changes behaviour (different output for same plugin options) ;)

[timhagn]

*hmmz* But it fixes behavior in a backwards compatible manner, or doesn't it? Just thinking according to SemVer that'd be a minor version update. But what's semver more than "semantics" - so let's not dive into them ; ). In the end it's up to you and the other maintainers anyway ^^.

[timhagn]

Anyone willing to give this a review, please : )?

[timhagn]

@marcysutton @KyleAMathews @DSchau
Is this still wanted and would anyone of you be willing to give it a review, please : )?
What do you think about @pieh's objection? Would be willing to add an option for old vs. "new" behavior in the meantime... *thinks*

[DSchau]

This seems pretty reasonable to me.

I think @pieh's reservation about a major bump (which is reasonable) is more that we can't guarantee this doesn't break something. While testing with reactjs.org is great (and thank you for doing it!), it doesn't guarantee nothing is broken. Since this is a possibly breaking change, I'd be fine releasing this as a major bump to just be on the safe side.

[timhagn]

Thanks for reviewing it, @DSchau!
As written, I've committed your suggested change to the README, hope it doesn't break the overall tests now - as you and the team seem to have made quite some progress while I re-opened this PR : ).
In case it would (at 15 successful ones now, fingers crossed ; ) and before I accidentally break the PR again, what's the preferred way of updating a PR to gatsbyjs from upstream? Merging changes into my local repo?
What do you think about me adding a switch in the meantime? And no prob about testing it with reactjs.org, was an interesting insight for me : ).
Thanks again!

[pieh]

what's the preferred way of updating a PR to gatsbyjs from upstream?

We do squash commits from PR, so merging gatsbyjs/gatsby master branch into is ok. Rebasing would work too. You can chose whatever workflow you are comfortable with. I'll update branch and do some local testing and hopefully merge this today.

I will do major version bump - this was never a blocker, just a note. We can bump plugins major versions when needed and this is not tied to Gatsby core package major versions bump which are far less frequent.

[pieh]

Thanks for patience on this. Changes looks good!

[gatsbot]

Holy buckets, @timhagn — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

[pieh]

Published gatsby-remark-embed-snippet@4.0.0

Also added some BREAKING CHANGES note in changelog - https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-remark-embed-snippet/CHANGELOG.md#400-2019-06-14

[timhagn]

Thanks a lot @pieh, was worth the wait ; ). And to you, too, of course, @DSchau! Let's just say the whole Gatsby team : )!