Skip to content

Conversation

@pecoram
Copy link
Contributor

@pecoram pecoram commented Jan 21, 2025

In chrome 38 if some attribute is undefined the font is not loaded

@pecoram pecoram requested a review from philippe-wm January 22, 2025 10:00
@chiefcll
Copy link
Contributor

I recommend removing the object.fromEntries as that usually doesnt get compiled correctly on older devices

@philippe-wm
Copy link

@chiefcll TBH a lot of features are missing on older devices, like Array.includes, so generally you must include some core-js polyfilling, ideally based on usage as Babel can do.

@wouterlucas
Copy link
Contributor

@chiefcll TBH a lot of features are missing on older devices, like Array.includes, so generally you must include some core-js polyfilling, ideally based on usage as Babel can do.

Although not wrong, if the idea is to make stuff work on Chrome v38 you can easily achieve the same with functionality that we know is available on chrome v38. Avoiding another place to polyfill and providing consistent behaviour, likely get some (albeit super minor) performance increases on the way.

I'm with Chris here, you can easily achieve the same with e.g. a for...in loop, write it to a new array and use that from there on (or replace the original one).

Copy link
Contributor

@wouterlucas wouterlucas left a comment

Choose a reason for hiding this comment

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

Please replace Object.fromEntries with a for loop or similar, to align with targeted chrome v38 functionality.

@pecoram pecoram requested a review from wouterlucas January 23, 2025 16:42
Copy link
Contributor

@wouterlucas wouterlucas left a comment

Choose a reason for hiding this comment

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

Thanks!

@wouterlucas wouterlucas added this pull request to the merge queue Jan 23, 2025
Merged via the queue into lightning-js:main with commit a34dbfc Jan 23, 2025
2 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.

5 participants