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

Add Glimmer to SUPPORTED_LANGUAGES #3123

Merged
merged 3 commits into from
Apr 13, 2021
Merged

Conversation

NullVoxPopuli
Copy link
Contributor

Adds Glimmer highlighting plugin to list of supported languages

@joshgoebel
Copy link
Member

joshgoebel commented Apr 10, 2021

Now I'm trying to figure out what to do (if anything) about the fact that you take the same aliases we ship... so installing your plugin REPLACES our built-in functionality... I think that's a first. Not sure if we need to disallow that (probably too drastic) or merely call it out, or what...

I assume you think that is a feature not a bug? :)

@joshgoebel joshgoebel changed the title Add Glimmer Add Glimmer to SUPPORTED_LANGUAGES Apr 10, 2021
@joshgoebel
Copy link
Member

I'm thinking since Handlebars still exists outside of Glimmer/Ember ecosystem that the aliases (by default) might not be correct. That someone should have to manually link them up if that's the behavior they desire.

@NullVoxPopuli
Copy link
Contributor Author

I assume you think that is a feature not a bug? :)

yes, a feature :D

The issue primarily comes from Glimmer files reusing the hbs extension.... but... 🤷

However,

so installing your plugin REPLACES our built-in functionality

My plugin doesn't actually register itself, you need to do that manually via:

import hljs from 'highlight.js';
import { glimmer } from 'highlightjs-glimmer';

hljs.registerLanguage('glimmer', glimmer);
hljs.highlightAll();

which, idk.. I could do in the plugin, but that's a side-effect, and side-effects are generally less flexible in broader ES modules (even though probably totally fine for how highlightjs is used)

@joshgoebel
Copy link
Member

My plugin doesn't actually register itself, you need to do that manually via:

You should also ship a CDN distributable in dist and it should self register as your name, ie glimmer. I'll create the same issue I filed for others recently.

Aliases always self-register though whenever the language is registered with registerLanguage.

The issue primarily comes from Glimmer files reusing the hbs extension

Which means that hbs is now ambiguous in meaning... which is problematic. I'm thinking perhaps your alias list should instead be empty and your README should show a one-liner for how someone could hook up those aliases if that is their desire - to replace those aliases?

Trying to avoid surprising behavior.

@NullVoxPopuli
Copy link
Contributor Author

You should also ship a CDN distributable in dist and

workin on that now 👍

Trying to avoid surprising behavior.

I get that -- and I understand that this may be surprising behavior to someone, but Glimmer is kind of a superset of handlebars, buuuuut, more importantly, I have a feeling the number of people in the world that would want both syntaxes is < 10

@joshgoebel
Copy link
Member

joshgoebel commented Apr 10, 2021

kind of a superset

kind of indeed. 😄 And then the question is why not go all the way and also clobber handlebars itself - if installing glimmer truly means "I want glimmer everywhere"? Or were you giving it a bit more lee way as it's the actual canonical name of our built-in grammar?

I think at a minimum your README should perhaps mention (as a "caveat") that it clobbers those aliases and someone would need to re-register them to point to handlebars if they did not desire that behavior. I'm not SUPER worried about this, but it is a precedent I think so it's worth a few brain neurons to try and do what's best, whatever we decide that is.

@allejo Any thoughts on this?

@NullVoxPopuli
Copy link
Contributor Author

actual canonical name of our built-in grammar?

yeah, moreso this. The term handlebars is rarely used in the Glimmer/Ember world, except when talking about the file extension itself.

I think at a minimum your README should perhaps mention

I did add this (very recently):

NOTE highlightjs-glimmer cannot be used at the same time as highlightjs' handlebars syntax highlighting.

@joshgoebel
Copy link
Member

NOTE highlightjs-glimmer cannot be used at the same time as highlightjs' handlebars syntax highlighting.

But that's wholly inaccurate (or at the very least imprecise) no? One can easily do:

<pre><code class="lang-glimmer">...</code></pre>
<pre><code class="lang-handlebars">...</code></pre>

So long as you're leaving handlebars alone (which I do think is correct), then this should work just fine. What you're breaking is the aliases, not the whole grammar.

@NullVoxPopuli
Copy link
Contributor Author

But that's wholly inaccurate (or at the very least imprecise) no? One can easily do:

👍 I'll update the verbiage: NullVoxPopuli/highlightjs-glimmer@4a239f2

@NullVoxPopuli
Copy link
Contributor Author

Success!
image
https://github.com/NullVoxPopuli/highlightjs-glimmer/runs/2315197032

@joshgoebel
Copy link
Member

👍 I'll update the verbiage: NullVoxPopuli/highlightjs-glimmer@4a239f2

@allejo Any objections? I'm "ok" after this README change I think.

@joshgoebel
Copy link
Member

Add to changes.md too please?

@NullVoxPopuli
Copy link
Contributor Author

added!

@joshgoebel joshgoebel merged commit 3cae42d into highlightjs:main Apr 13, 2021
@joshgoebel
Copy link
Member

@NullVoxPopuli Thanks!

@NullVoxPopuli NullVoxPopuli deleted the patch-1 branch April 13, 2021 22:43
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.

None yet

2 participants