-
Notifications
You must be signed in to change notification settings - Fork 9
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
chore(deps): upgrading @readme/syntax-highlighter
#61
Conversation
@@ -4,8 +4,8 @@ const PropTypes = require('prop-types'); | |||
// Only load CodeMirror in the browser, for SSR | |||
// apps. Necessary because of people like this: | |||
// https://github.com/codemirror/CodeMirror/issues/3701#issuecomment-164904534 | |||
const syntaxHighlighter = typeof window !== 'undefined' ? require('@readme/syntax-highlighter') : false; | |||
const canonicalLanguage = require('@readme/syntax-highlighter/canonical'); | |||
const syntaxHighlighter = typeof window !== 'undefined' ? require('@readme/syntax-highlighter').default : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the server-compatible version of this, index.node.js
is the main
entrypoint, you might need to reference the client version.
Here's main
: https://github.com/readmeio/syntax-highlighter/blob/master/src/index.node.js (no default).
Here's the client: https://github.com/readmeio/syntax-highlighter/blob/master/src/index.js (default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what I need to change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kellyjosephprice You mind taking this? I'm not sure what exactly this require needs to be changed to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there's no change needed? It looks like syntax-highlighter is using the browser
field in package.json
and the webpack build for markdown is loading the correct file.
$ rg SyntaxHighlighter dist
dist/main.node.js
5057: }), console.log("SyntaxHighlighter loading server");
dist/main.js
20852: console.log("SyntaxHighlighter loading client");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @erunion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulling back here as I just checked this out to verify, and it looks like there are some SSR issues per Gabe's concerns. (To be fair though, I'm using npm link
locally so, as always, I'm a touch skeptical as to how well this replicates a "true" install environment...)
[0] webpack:///./node_modules/codemirror/lib/codemirror.js?:17
[0] var userAgent = navigator.userAgent;
Full Trace
[0] ReferenceError: navigator is not defined
[0] at eval (webpack:///codemirror/lib/codemirror.js?:17:19)
[0] at Object../node_modules/codemirror/lib/codemirror.js (/markdown/dist/main.node.js:510:1)
[0] at __webpack_require__ (/Users/rafegoldberg/Repos/ReadMe/markdown/dist/main.node.js:21:30)
[0] at eval (webpack:///./node_modules/@readme/syntax-highlighter/codemirror.jsx?:13:18)
[0] at Object../node_modules/@readme/syntax-highlighter/codemirror.jsx (/Users/rafegoldberg/Repos/ReadMe/markdown/dist/main.node.js:310:1)
[0] at __webpack_require__ (/Users/rafegoldberg/Repos/ReadMe/markdown/dist/main.node.js:21:30)
[0] at eval (webpack:///./node_modules/@readme/syntax-highlighter/index.js?:3:18)
[0] at Object../node_modules/@readme/syntax-highlighter/index.js (/Users/rafegoldberg/Repos/ReadMe/markdown/dist/main.node.js:321:1)
[0] at __webpack_require__ (/Users/rafegoldberg/Repos/ReadMe/markdown/dist/main.node.js:21:30)
[0] at eval (webpack:///./components/Code/index.jsx?:12:16)
[0] at Object../components/Code/index.jsx (/Users/rafegoldberg/Repos/ReadMe/markdown/dist/main.node.js:119:1)
[0] at __webpack_require__ (/Users/rafegoldberg/Repos/ReadMe/markdown/dist/main.node.js:21:30)
[0] at eval (webpack:///./components/index.js?:5:63)
[0] at Module../components/index.js (/Users/rafegoldberg/Repos/ReadMe/markdown/dist/main.node.js:230:1)
I can look a bit closer, unless you want to take this one @kellyjosephprice?
Yea, looking now. |
So actually, I may have been on an outdated branch when I got that error ‘cause when I run this against next it's working. It does look like these highlighter changes caused some minor problems rendering inline code snippets, though. Update: whelp, that would actually break variable expansion in inline code snippets... The underlying issue is that the updated highlighter now wraps code with a block-level markdown/components/Code/style.scss Lines 30 to 32 in 8b88a40
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM
This PR was released!🚀 Changes included in v6.22.1 |
🧰 Changes
Been a while since the syntax highlighter has been updated here and since I added support for TOML to it today, we should get it up to date here so Markdown code blocks with that language should get highlighted.
📖 Release Notes
10.2.0 (2020-10-29)
10.1.2 (2020-10-26)
10.1.1 (2020-10-23)
10.1.0 (2020-10-22)
10.0.1 (2020-10-12)
10.0.0 (2020-10-05)
9.0.1 (2020-09-16)
9.0.0 (2020-09-15)
8.0.3 (2020-09-02)
8.0.2 (2020-09-02)
8.0.1 (2020-09-01)
8.0.0 (2020-08-31)