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

fix(parser) Make language alias registration case insensitive #3026

Merged
merged 5 commits into from Mar 2, 2021

Conversation

davido
Copy link
Contributor

@davido davido commented Feb 28, 2021

Changes

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md
  • Added myself to AUTHORS.txt, under Contributors

@davido
Copy link
Contributor Author

davido commented Feb 28, 2021

Upstream discussion is here: [1].

[1] https://crbug.com/gerrit/14091

@joshgoebel
Copy link
Member

This will not fix: Could not find the language 'latex', did you forget to load/include a language module?

The problem is that your installation of Highlight.js is not loading the latex language module (as the error is telling you). Adding all the aliases in the world will not fix the issue. Latex is not including in the "common" distributable so you'll need to build a custom build or add additional JS files (see our README for directions).

It's possible you think you've fixed it locally because you're using a different build of the library than is being used in production? But that error would not be shown if latex was loaded and registered.


This might still be a reasonable PR but I'm going to briefly noodle over whether we simply shouldn't make aliases case-insensitive rather than having this type of issue continue to come up.

@davido
Copy link
Contributor Author

davido commented Feb 28, 2021

This will not fix: Could not find the language 'latex', did you forget to load/include a language module?

I have not experienced this issue.

I debugged gerrit@HEAD and it turned out, that this hljs function:

      /**
       * @param {string} name - name of the language to retrieve
       * @returns {Language | undefined}
       */
      function getLanguage(name) {
        name = (name || '').toLowerCase();
        return languages[name] || languages[aliases[name]];
      }

returned undefined for tex language name.

The problem is that your installation of Highlight.js is not loading the latex language module (as the error is telling you). Adding all the aliases in the world will not fix the issue. Latex is not including in the "common" distributable so you'll need to build a custom build or add additional JS files (see our README for directions).

Gerrit build instruction for hljs is simple:

    $>  # start in some temp directory
    $>  git clone https://github.com/highlightjs/highlight.js
    $>  cd highlight.js
    $>  patch -p1 < ../0001-Add-tex-alias-to-latex-mode.patch
    $>  git clone https://github.com/highlightjs/highlightjs-closure-templates
    $>  ln -s ../../highlightjs-closure-templates/soy.js src/languages/soy.js
    $>  mkdir test/detect/soy && ln -s ../../../highlightjs-closure-templates/test/detect/soy/default.txt test/detect/soy/default.txt
    $>  npm install
    $>  node tools/build.js

Now, grepping for latex in build/highlight.min.js produced these matches:

  $ grep -i latex build/highlight.min.js
  hljs.registerLanguage("latex",(()=>{"use strict";return e=>{const n=[{
  });return{name:"LaTeX",aliases:["tex","TeX"],

With the diff from this PR the highlighting of LaTeX sources with .tex extension just work.

Also in https://github.com/highlightjs/highlight.js/blob/master/SUPPORTED_LANGUAGES.md no additional packages are listed for LaTeX mode.

What am I missing?

@joshgoebel
Copy link
Member

joshgoebel commented Feb 28, 2021

I have not experienced this issue.

Ah, ok we're talking about different issues then.

[getLanguage] returned undefined for tex language name.

Ah yes, thank for running this down. :-)

name = (name || '').toLowerCase();

Ugh. So we're already normalizing aliases at request/runtime, but not at registration time. I think the fix we truly need here is in registerAliases on line ~878 instead:

aliasList.forEach(alias => { aliases[alias.toLowerCase()] = languageName; });

If the lookup is downcase, then the registration should be also. Could you confirm if this also resolves your issue and perhaps update the PR if so?

@davido davido changed the title Add tex alias to latex mode Make language registration case insensitive Mar 1, 2021
@davido
Copy link
Contributor Author

davido commented Mar 1, 2021

Could you confirm if this also resolves your issue

Yes.

[...] and perhaps update the PR if so?

Done.

@joshgoebel joshgoebel changed the title Make language registration case insensitive fix(parser) Make language alias registration case insensitive Mar 1, 2021
@joshgoebel
Copy link
Member

Please give yourself some credit in CHANGES.md. :) Otherwise looks good.

@davido
Copy link
Contributor Author

davido commented Mar 2, 2021

Please give yourself some credit in CHANGES.md.

Done.

CHANGES.md Outdated Show resolved Hide resolved
@joshgoebel joshgoebel merged commit d24895f into highlightjs:master Mar 2, 2021
@joshgoebel
Copy link
Member

@davido Thanks!

@davido davido deleted the add_tex_alias_to_latex_mode branch March 2, 2021 07:34
lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this pull request Mar 5, 2021
Add tex files to gr-syntax-layer's language map. Also update hljs to
d24895f4 to include this fix: [1]

  fix(parser) Make language alias registration case insensitive

[1] highlightjs/highlight.js#3026

Bug: Issue 14091
Change-Id: I41f855703f0a9d70b6aebd2a337f1c4c7bf91904
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