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/issue 3972 #3989

Merged
merged 8 commits into from
Feb 25, 2024
Merged

Fix/issue 3972 #3989

merged 8 commits into from
Feb 25, 2024

Conversation

akhtarmdsaad
Copy link
Contributor

@akhtarmdsaad akhtarmdsaad commented Feb 18, 2024

Problem: When I use namespace as the name of the variable, everything that follows is highlighted incorrectly. While namespace is a TS keyword it's fine to use it as a variable name too.
Solution: Used a multimatcher to solve it

Before:

image

After:

image

Resolves #3972

Changes

Before:

const NAMESPACE = {
    beginKeywords: 'namespace',
    end: /\{/,
    excludeEnd: true,
    contains: [ tsLanguage.exports.CLASS_REFERENCE ]
  };

After:

const NAMESPACE = {
    begin: [
      /namespace/,
      /\s+/,
      hljs.IDENT_RE
    ],
    beginScope: {
      1: "keyword",
      3: "variable"
    },
    end: /\{/,
    excludeEnd: true,
    contains: [tsLanguage.exports.CLASS_REFERENCE]
  };

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

5 files changed

Total change +40 B

View Changes
file base pr diff
es/core.min.js 8.17 KB 8.17 KB -1 B
es/highlight.min.js 8.17 KB 8.17 KB -1 B
es/languages/typescript.min.js 3.15 KB 3.17 KB +22 B
highlight.min.js 8.21 KB 8.2 KB -2 B
languages/typescript.min.js 3.16 KB 3.18 KB +22 B

],
beginScope: {
1: "keyword",
3: "variable"
Copy link
Member

Choose a reason for hiding this comment

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

From the examples (https://www.typescriptlang.org/docs/handbook/namespaces.html) it looks like namespaces are class-like and CamelCase in their naming... so I think title.class would be more appropriate here than variable.

Comment on lines 40 to 42
end: /\{/,
excludeEnd: true,
contains: [ tsLanguage.exports.CLASS_REFERENCE ]
contains: [tsLanguage.exports.CLASS_REFERENCE]
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need any of this anymore - the match rule should be enough to highlight the keyword and namespace name... but should IDENT_RE used above to recognize namespace perhaps be CLASS_REFERENCE[:match] instead - if the same names rules apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const NAMESPACE = {
    begin: [
      /namespace/,
      /\s+/,
      hljs.IDENT_RE
    ],
    beginScope: {
      1: "keyword",
      3: "title.class"
    }
  };

Is the above change enough or i should change something else.

Copy link
Member

Choose a reason for hiding this comment

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

I think I was suggesting tsLanguage.exports.CLASS_REFERENCE instead of IDENT_RE... but yes that's looking good.

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

5 files changed

Total change +31 B

View Changes
file base pr diff
es/core.min.js 8.17 KB 8.18 KB +2 B
es/highlight.min.js 8.17 KB 8.18 KB +2 B
es/languages/typescript.min.js 3.15 KB 3.16 KB +12 B
highlight.min.js 8.21 KB 8.21 KB +2 B
languages/typescript.min.js 3.16 KB 3.17 KB +13 B

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

3 files changed

Total change +24 B

View Changes
file base pr diff
es/languages/typescript.min.js 3.15 KB 3.16 KB +12 B
highlight.min.js 8.21 KB 8.21 KB -1 B
languages/typescript.min.js 3.16 KB 3.17 KB +13 B

@@ -0,0 +1,4 @@

<span class="hljs-keyword">const</span> message = <span class="hljs-string">&#x27;foo&#x27;</span>;
<span class="hljs-keyword">const</span> <span class="hljs-keyword">namespace</span> = <span class="hljs-string">&#x27;bar&#x27;</span>;
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the desire behavior... We'll also need to remove it from TS_SPECIFIC_KEYWORDS (lets just comment it out - and mention it has a separate rule below).

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

3 files changed

Total change +12 B

View Changes
file base pr diff
es/languages/typescript.min.js 3.15 KB 3.16 KB +6 B
highlight.min.js 8.21 KB 8.21 KB -1 B
languages/typescript.min.js 3.16 KB 3.17 KB +7 B

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

3 files changed

Total change +12 B

View Changes
file base pr diff
es/languages/typescript.min.js 3.15 KB 3.16 KB +6 B
highlight.min.js 8.21 KB 8.21 KB -1 B
languages/typescript.min.js 3.16 KB 3.17 KB +7 B

@joshgoebel joshgoebel merged commit 607a929 into highlightjs:main Feb 25, 2024
15 checks passed
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.

(typescript) Using namespace as a variable name breaks highlighter state
2 participants