Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Propose: Improve highlight API #2277

Closed
6 of 8 tasks
joshgoebel opened this issue Nov 12, 2019 · 20 comments · Fixed by #3053
Closed
6 of 8 tasks

Propose: Improve highlight API #2277

joshgoebel opened this issue Nov 12, 2019 · 20 comments · Fixed by #3053
Labels
discuss/propose Proposal for a new feature/direction enhancement An enhancement or new feature parser
Milestone

Comments

@joshgoebel
Copy link
Member

joshgoebel commented Nov 12, 2019

For those finding there way here from the deprecation warning it's easy to upgrade your code:

// The old API:
highlight(language, code, ignore_illegals, continuation) {
// The new API take an options object literal:
highlight(code, { language, ignoreIllegals}) {

So to upgrade your code:

// before
html = highlight("vbscript","your code goes here", true)
// after
html = highlight("your code goes here", {language: "vbscript", ignoreIllegals: true })

IE, just switch the order of the arguments and pass language and ignoreIllegals in the options object now.


Advantages:

  • target of the verb [highlight] is now the first argument, which makes more sense, such as format(drive), highlight(code)
  • no need to remember order of the arguments
  • we can still support the old API for legacy (ie, non-breaking change)
  • fewer arguments are generally better (easier to understand, now at the call site you always see the names of the optional arguments, etc)

Checklist:

@joshgoebel joshgoebel added enhancement An enhancement or new feature parser labels Nov 12, 2019
@joshgoebel joshgoebel added this to the 10.0 milestone Nov 12, 2019
@joshgoebel joshgoebel changed the title Improve highlight API Suggest: Improve highlight API Nov 12, 2019
@egor-rogov
Copy link
Collaborator

Yes, the suggested API looks better to my taste. But I can't see enough value in it to justify API breaking change.
Making it non-breaking - well, why not.

@joshgoebel joshgoebel modified the milestones: 10.0, 10.1 Dec 24, 2019
@joshgoebel joshgoebel removed this from the 10.1 milestone Apr 21, 2020
@joshgoebel joshgoebel added the discuss/propose Proposal for a new feature/direction label Apr 30, 2020
@joshgoebel
Copy link
Member Author

Any objection to:

  • highlightBlock -> highlightElement
  • initHighlighting -> start() or highlightAll()
  • initHighlightingOnLoad -> startOnLoad()

Both for clarity and simplicity.

@egor-rogov
Copy link
Collaborator

start doesn't looks clear to me, highlightAll is better to my taste.

@joshgoebel joshgoebel changed the title Suggest: Improve highlight API Propose: Improve highlight API May 16, 2020
@joshgoebel joshgoebel added this to the 11.0 milestone Jan 7, 2021
@joshgoebel
Copy link
Member Author

@allejo Any thoughts on these API naming changes? I think this was a topic before we added you to the team.

@allejo
Copy link
Member

allejo commented Jan 14, 2021

I agree with Egor where "start" isn't clear. I'd lean more towards:

  • highlightDOMElement or highlightDOMNode (I don't immediately think a DOM element if it's just highlightElement)
  • highlightAll
  • highlightAllOnLoad

@joshgoebel
Copy link
Member Author

joshgoebel commented Jan 14, 2021

After more though I don't like this highlightAllOnLoad vs highlightAll nonsense and forcing users to think about WHEN. I think instead:

highlightAll() - just works, no matter when it's called (hooks onload, or if already loaded simply highlights)

A single API.

@joshgoebel
Copy link
Member Author

I think I'm going to leave highlightAuto for now since change that gets into issues with callback names and other things...

laurent22 added a commit to laurent22/joplin that referenced this issue Oct 4, 2021
runchard pushed a commit to runchard/joplin that referenced this issue Oct 17, 2021
foxxyz pushed a commit to foxxyz/node that referenced this issue Oct 18, 2021
- Migrated to ESM because some dependencies now require it.
- Did not update `highlight.js` to v11 because it has many breaking
  changes.
- Used non-deprecated `highlight.js` API.

Refs: highlightjs/highlight.js#2277
Fixes: nodejs#38938
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs#38966
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
alesandroortiz added a commit to alesandroortiz/eleventy-plugin-highlightjs that referenced this issue Nov 12, 2021
smnscp added a commit to smnscp/similitude-hugo that referenced this issue Feb 3, 2022
* Remove deprecated Linkify option.
* Replace deprecated Highlight syntax.
  (highlightjs/highlight.js#2277)
CHENXCHEN pushed a commit to CHENXCHEN/hexo-renderer-markdown-it-plus that referenced this issue Aug 8, 2022
Deprecated as of 10.7.0. highlight(lang, code, ...args) has been deprecated.
Please use highlight(code, options) instead.
highlightjs/highlight.js#2277
@precondition
Copy link

I appreciate the quick rundown on how to upgrade the code.

clarkf added a commit to clarkf/matrix-react-sdk that referenced this issue Jan 13, 2023
Previous call signature was deprecated.

See highlightjs/highlight.js#2277

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>
clarkf added a commit to clarkf/matrix-react-sdk that referenced this issue Jan 14, 2023
Previous call signature was deprecated.

See highlightjs/highlight.js#2277

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>
clarkf added a commit to clarkf/matrix-react-sdk that referenced this issue Jan 14, 2023
Previous call signature was deprecated.

See highlightjs/highlight.js#2277

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>
clarkf added a commit to clarkf/matrix-react-sdk that referenced this issue Jan 14, 2023
Previous call signature was deprecated.

See highlightjs/highlight.js#2277

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>
SimonBrandner pushed a commit to matrix-org/matrix-react-sdk that referenced this issue Jan 15, 2023
* Fix viewing source of redacted events

Clicking 'View Source' in the context menu of a redacted event causes an error,
and the user gets no visible result.

This fixes <ViewSource /> to indicate that the source is unavailable when a
message has been redacted. The original source remains available.

<SyntaxHighlight /> requires a non-null string for its `content` prop, and, in
the case of redacted events, <ViewSource /> was passing `undefined`. This is
ultimately because redacting an event causes `MatrixEvent.clearEvent` to be
`undefined`, which <ViewSource /> wasn't checking.

Fixes element-hq/element-web#24165

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>

* Use correct highlight.js call

Previous call signature was deprecated.

See highlightjs/highlight.js#2277

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>
srix added a commit to srix/prog-lang-compare that referenced this issue May 20, 2023
espadrine added a commit to espadrine/latexmarkdown that referenced this issue Jul 14, 2023
Following the dependency upgrade, when running the tests,
the highlight.js dependency printed this warning:

> Deprecated as of 10.7.0. highlight(lang, code, ...args) has been deprecated.
> Deprecated as of 10.7.0. Please use highlight(code, options) instead.
> highlightjs/highlight.js#2277

We address the deprecation by switching the call to the new form.
@176851899

This comment was marked as spam.

@176851899

This comment was marked as spam.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
discuss/propose Proposal for a new feature/direction enhancement An enhancement or new feature parser
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants