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

File Highlight & JSONP Highlight update #1974

Merged

Conversation

RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Jul 12, 2019

This implements the changes to File Highlight discussed here and also applies them to JSONP Highlight making both APIs very similar.

I took the chance to basically rewrite both plugins, so here's a list of changes:

  • Both now add special attributes to the pre element depending on the current status. The attributes are data-{src|jsonp}-status="{loading|loaded|failed}" depending on the plugin and the current status. These also prevent multiple (or even concurrent) executions for the same element.
    Edit: changed attributes
  • It adds a new restriction :empty for all pre elements being processed by the plugins.
    Why? It doesn't make any sense for the pre to contain anything, so enforcing it seems like a good idea (and a breaking change).
    The previous behavior was that FH just delete the text and JH just appended the new code element without doing anything.

    Edit: Removed for backward compatibility.
  • Deprecated Prism.fileHighlight in favor of Prism.plugins.fileHighlight.highlight analog to Prism.plugins.jsonphighlight.highlight.
  • Added a timeout config property for JSONP Highlight.
  • Updated error messages of both plugins to be more consistent.
  • FH preloads Both preload languages using the Autoloader if available.
  • Resolves the plugin incompatibility between FH and Copy to clipboard.

A few things I would like to further discuss and potentially change:

1. Preloading language. Edit: Done.
Both plugins know which language the code is before the file is received. This can be used to preload the language definition using the Autoloader which has to fetch it anyway.
This requires exposing Autoloader's language loading API (already done in #1898). Alternatively, we could also create a fake element to highlight which will cause Autoloader to load the languages.

2. Fixing callbacks. Edit: The fix in #1973 is good enough
Right now, the callback function for the pres won't be executed at all. #1973 fixes this but only calls it synchronously.
To fix this I would like to add a new hook: delegate. The purpose of the hook is to allow highlightElement to hand off the responsibility of highlighting the given element to some other instance. All registered hook callbacks will then be asked whether they take the element and the first to say yes has the responsibility to handle highlighting the element (including calling the callback).
The environment of the hook I imagine like this:

interface DelegateHookEnvironment {
    element: Element;
    async: boolean;
    callback?: (element: Element) => void;
    language: string;
    delegate: boolean;
}

Hook callbacks will set the delegate property to true to say yes.
The hooks will be run before any DOM mutation, so here and the code to do so could look like this:

const env = { element, async, callback, language, delegate: false };
for (const f of Prism.hooks.all['delegate']) {
    f(env);
    if (env.delegate) return;
}

Thoughts?

3. Asynchronous highlighting. Edit: Maybe later.
Right now, both FH & JH ignore the async parameter and highlight code synchronously.
Note: The proposed solution for 2) fixes this issue as well.


This resolves #1965.

@RunDevelopment
Copy link
Member Author

RunDevelopment commented Jan 10, 2020

I added this to the PR.

It's pretty much ready as-is aside from one detail:
The :empty pseudo-class added to the selector is a breaking change. I understand why it makes sense to enforce this but is it worth a breaking change?

I removed the :emtpy check.

@RunDevelopment
Copy link
Member Author

@mAAdhaTTah
I showed this PR some love again and would like to merge this soon (because both #1813 and #1969 are blocked by this one).

As a note:
The before-sanity-check hook of both plugins is quite similar because I wanted them to have a somewhat similar API. Despite that, I don't think that there is much to gain from trying to abstract and moving the common elements into a separate plugin.

Copy link
Member

@mAAdhaTTah mAAdhaTTah left a comment

Choose a reason for hiding this comment

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

If my understanding of that coordination is correct, then this looks good to me.

@RunDevelopment RunDevelopment merged commit afea17d into PrismJS:master Jun 28, 2020
@RunDevelopment RunDevelopment deleted the file-and-jsonp-highlight-update branch June 28, 2020 00:33
quentinvernot pushed a commit to TankerHQ/prismjs that referenced this pull request Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugins incompatibility: File Highlight and Copy to Clipboard
2 participants