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(typescript) better highlighting of optionals #4114

Merged
merged 5 commits into from
Sep 29, 2024

Conversation

Dxuian
Copy link
Contributor

@Dxuian Dxuian commented Sep 9, 2024

Fixed the optional property in ts not being highlighted
Resolves #3613
before:
image

after:
image

Checklist

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

Copy link

github-actions bot commented Sep 9, 2024

Build Size Report

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

7 files changed

Total change +71 B

View Changes
file base pr diff
es/core.min.js 8.18 KB 8.18 KB +1 B
es/highlight.min.js 8.18 KB 8.18 KB +1 B
es/languages/javascript.min.js 2.73 KB 2.74 KB +4 B
es/languages/typescript.min.js 3.17 KB 3.2 KB +29 B
highlight.min.js 8.22 KB 8.22 KB +2 B
languages/javascript.min.js 2.74 KB 2.74 KB +4 B
languages/typescript.min.js 3.18 KB 3.21 KB +30 B

@@ -1,4 +1,4 @@
export const IDENT_RE = '[A-Za-z$_][0-9A-Za-z$_]*';
export const IDENT_RE = '[A-Za-z$_][0-9A-Za-z$_]*\\??';
Copy link
Member

Choose a reason for hiding this comment

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

Is ? truly a valid ECMAScript identifier, or just in TS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Youre right , ive managed to scope the new fix to just ts in the new commit

const namespace = 'bar';
*/

const OPTIONAL_PROPERTY_RE = IDENT_RE + '\\s*\\?';
Copy link
Member

Choose a reason for hiding this comment

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

Please provide an example of what this is trying to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in my new commit ive added some test so i found that it was failing on some new tcs so ill explain the new one

Copy link

Build Size Report

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

2 files changed

Total change +74 B

View Changes
file base pr diff
es/languages/typescript.min.js 3.17 KB 3.21 KB +37 B
languages/typescript.min.js 3.18 KB 3.21 KB +37 B

Copy link

Build Size Report

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

7 files changed

Total change +79 B

View Changes
file base pr diff
es/core.min.js 8.18 KB 8.18 KB -2 B
es/highlight.min.js 8.18 KB 8.18 KB -2 B
es/languages/javascript.min.js 2.73 KB 2.74 KB +4 B
es/languages/typescript.min.js 3.17 KB 3.21 KB +37 B
highlight.min.js 8.22 KB 8.22 KB +1 B
languages/javascript.min.js 2.74 KB 2.74 KB +4 B
languages/typescript.min.js 3.18 KB 3.21 KB +37 B

Copy link

Build Size Report

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

7 files changed

Total change +79 B

View Changes
file base pr diff
es/core.min.js 8.18 KB 8.18 KB -2 B
es/highlight.min.js 8.18 KB 8.18 KB -2 B
es/languages/javascript.min.js 2.73 KB 2.74 KB +4 B
es/languages/typescript.min.js 3.17 KB 3.21 KB +37 B
highlight.min.js 8.22 KB 8.22 KB +1 B
languages/javascript.min.js 2.74 KB 2.74 KB +4 B
languages/typescript.min.js 3.18 KB 3.21 KB +37 B

@joshgoebel
Copy link
Member

I rewrote this to use more of the existing attr rule. All tests still seem to pass. See any issues?

@joshgoebel joshgoebel changed the title fixed optional property not highlighted correctly in ts fix(typescript) better highlighting of optionals Sep 22, 2024
@Dxuian
Copy link
Contributor Author

Dxuian commented Sep 23, 2024

I rewrote this to use more of the existing attr rule. All tests still seem to pass. See any issues?

its highlighting wrong
image
before:
image

i dont see what seems to be the issue in the previous commit i made can you ellaborate a little more

@joshgoebel
Copy link
Member

its highlighting wrong

The tests [you provided] (including the one you show above) are all still green... given that the tests are all green, my version has the exact same behavior as yours. So... not sure what you are seeing.

# ./tools/build.js -t node && npm run test-markup
...
 531 passing (1s)

i dont see what seems to be the issue in the previous commit i made

It's just a bit too complex... we usually try for the simplest solutions.

@Dxuian
Copy link
Contributor Author

Dxuian commented Sep 29, 2024

its highlighting wrong

The tests [you provided] (including the one you show above) are all still green... given that the tests are all green, my version has the exact same behavior as yours. So... not sure what you are seeing.

# ./tools/build.js -t node && npm run test-markup
...
 531 passing (1s)

i dont see what seems to be the issue in the previous commit i made

It's just a bit too complex... we usually try for the simplest solutions.

ah my apologies i changed to a different style and thought comment were being highlighted wrong
this seems to work fine can you merge and close this issue ?

@joshgoebel joshgoebel merged commit 0af0968 into highlightjs:main Sep 29, 2024
19 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) optional property not highlighted correctly
2 participants