-
Notifications
You must be signed in to change notification settings - Fork 852
Upgrade non-breaking JS dependencies #14152
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
Changes from all commits
f7eab2f
0c17fec
ee12551
ffa547b
b7b79b7
ecbf83f
c4bd190
5ec9a57
eda55b9
ca49a64
73688d6
601e3ed
5cfcae5
0f651e0
cc410a2
62d27b2
2f95be1
4114ad1
19ac18c
262c08f
10ee09d
a12ff8e
3ec2e73
a011a16
35de614
26a6bc9
b1e8e67
d004dae
64b4e67
74ee2c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,15 +63,15 @@ function normalize(lang) { | |
return MAP[lower] || lower; | ||
} | ||
|
||
function highlight(lang, code) { | ||
if(!lang) { | ||
function highlight(language, code) { | ||
if(!language) { | ||
return code; | ||
} | ||
// Normalize lang | ||
lang = normalize(lang); | ||
// Normalize language | ||
language = normalize(language); | ||
|
||
try { | ||
return hljs.highlight(lang, code).value; | ||
return hljs.highlight(code, {language}).value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why we're bundling highlight.js given the docs live in their own repo, I think? The API change would be breaking but the old API is still supported with deprecation warnings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IDK, we should ask @dothebart There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not related to your changes @pluma , but this file does contain lots of methods, variables and modules (like fs) which are not even used here. e.g. methods: logCurlRequestPlain / logCurlRequest / curlRequest There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix: Include those files in our JSLINT script :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I had one wish it's throwing out all our jslint nonsense and just using an external eslint+prettier via npm. We could probably also do this automatically for PRs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I bet our jslint rules aren't the best. Personally I do not care which tool we're going to use. Also adding jslint support for documentation related JS files is out of scope here. But please make sure that this will not be forgotten. @Simran-B fyi. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Simran-B do you have any insight what this documentation file does and why we bundle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To me it looks like the file is used in |
||
} catch(e) { } | ||
|
||
return code; | ||
|
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.
Oops.