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

trycatch font parsing in ligatures addon #3618

Closed
wants to merge 1 commit into from

Conversation

LabhanshAgrawal
Copy link
Contributor

@LabhanshAgrawal LabhanshAgrawal commented Jan 29, 2022

Adding the fix for #3547 to ligatures addon directly so that it can be used with older xterm.js versions
Ref #3617

@LabhanshAgrawal
Copy link
Contributor Author

LabhanshAgrawal commented Jan 29, 2022

Tests failing on

it('fails if it finds but cannot load the font', async () => {

and

it('ensures no empty errors are thrown', async () => {

As these expect errors being thrown which this pr suppresses, any suggestions?

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

to ligatures addon directly so that it can be used with older xterm.js versions

@LabhanshAgrawal isn't the right fix here to use the correct ligatures addon for the xterm.js version and update xterm.js if it's not fixed before? Throwing here and using the existing catch does seem like the more correct thing to do (instead of having a catch in xterm that would never be hit).

@Tyriar
Copy link
Member

Tyriar commented Feb 3, 2022

Closing out since #3617 can now be fixed by updating to latest webgl beta

@Tyriar Tyriar closed this Feb 3, 2022
@LabhanshAgrawal
Copy link
Contributor Author

@LabhanshAgrawal isn't the right fix here to use the correct ligatures addon for the xterm.js version and update xterm.js if it's not fixed before?

The issue was due to opentype not being able to parse some font on macOS 12, so none of the earlier ligatures addon versions would be able to work in that scenario even with previous xterm versions.

Throwing here and using the existing catch does seem like the more correct thing to do (instead of having a catch in xterm that would never be hit).

Didn't really get what you're referring to here.

Closing out since #3617 can now be fixed by updating to latest webgl beta

👍

@Tyriar
Copy link
Member

Tyriar commented Feb 4, 2022

Didn't really get what you're referring to here.

Would just prefer not to have try/catches everywhere when only one of them should be used in practice. We prefer to take a more evergreen approach as supporting older versions takes a bunch of effort.

@LabhanshAgrawal LabhanshAgrawal deleted the trycatch branch February 4, 2022 17:48
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.

2 participants