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

Use Prismjs dependency info from library instead of vendored version #5000

Merged
merged 2 commits into from
Apr 16, 2018
Merged

Use Prismjs dependency info from library instead of vendored version #5000

merged 2 commits into from
Apr 16, 2018

Conversation

rbayliss
Copy link
Contributor

@rbayliss rbayliss commented Apr 16, 2018

gatsby-remark-prismjs has historically used a vendored copy of PrismJS's language dependencies, because this data wasn't exposed on the PrismJS side. This is no longer the case - as of 1.13.0, language dependency data is exposed in components.js. This PR uses the data from components.js to load languages, meaning there will be no more maintenance required to stay current with PrismJS changes.

Fixes #5001

@gatsbybot
Copy link
Collaborator

Deploy preview for using-drupal ready!

Built with commit 4dbb56f3f6bb07aafaa703f18d161ce60965dfd3

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

@gatsbybot
Copy link
Collaborator

Deploy preview for using-drupal ready!

Built with commit 27ddff316c4a4d473b294fb366ef9c0dda4f1b3b

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

@gatsbybot
Copy link
Collaborator

Deploy preview for gatsbygram ready!

Built with commit 4dbb56f3f6bb07aafaa703f18d161ce60965dfd3

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

@gatsbybot
Copy link
Collaborator

Deploy preview for gatsbygram ready!

Built with commit 27ddff316c4a4d473b294fb366ef9c0dda4f1b3b

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

@gatsbybot
Copy link
Collaborator

gatsbybot commented Apr 16, 2018

Deploy preview for gatsbygram ready!

Built with commit 7935568

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

@gatsbybot
Copy link
Collaborator

gatsbybot commented Apr 16, 2018

Deploy preview for using-drupal ready!

Built with commit 7935568

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

@pieh
Copy link
Contributor

pieh commented Apr 16, 2018

Couldn't prismjs be peerDependency then? This update still ties prismjs version to gatsby-remark-prismjs. If prismjs would become peerDependency it would be up to user to pick prismjs version and even less maintenance needed for gatsby plugin. (just thinking here)

@rbayliss
Copy link
Contributor Author

I don't have an opinion there. As long as we're using prismJS > 1.13.0, whatever version of Prism should be fine. Would peerDependency require the user to install PrismJS explicitly, or would it be installed automatically still? (ie: Would the instructions need to change to yarn add gatsby-remark-prismjs prismjs?)

@pieh
Copy link
Contributor

pieh commented Apr 16, 2018

User would have to explicitly install prismjs - we already do this with react helmet plugin ( https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-plugin-react-helmet#install )

Copy link
Contributor

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah let's move to requiring PrismJS as a peerDependency, this is a pattern that's been adopted by other plugins too.

e.g. see https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-plugin-styled-components

@rbayliss
Copy link
Contributor Author

Ok, will do shortly.

@pieh
Copy link
Contributor

pieh commented Apr 16, 2018

Thanks for adding it to devDeps too. I've missed that this would cause issues with tests.

Last things from me:

  • could you bump gatsby-remark-prismjs version to 2.0.0 as this is breaking change (requiring to install prismjs package seperately)
  • update installation instruction in readme and add prismjs to npm install command there?

Signed-off-by: Rob Bayliss <rob@lastcallmedia.com>
@rbayliss
Copy link
Contributor Author

Ok, think we're all set with the requested changes. Sorry for the commit noise... having some trouble remembering to -s on my commits.

@pieh
Copy link
Contributor

pieh commented Apr 16, 2018

Don't worry about the noise. One snapshot is missing update - I guess this change got lost in all the signoff handling, because that test was passing before change to peerDeps.

Signed-off-by: Rob Bayliss <rob@lastcallmedia.com>
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for the PR! Always glad when it gets simpler to support an upstream :-)

@KyleAMathews KyleAMathews dismissed m-allanson’s stale review April 16, 2018 18:44

prismjs now moved to peerDependencies

@KyleAMathews KyleAMathews merged commit db7c8e3 into gatsbyjs:master Apr 16, 2018
@KyleAMathews
Copy link
Contributor

Oh, and congrats on snagging issue #5000!!! :-D

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

Successfully merging this pull request may close these issues.

5 participants