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 reference error for certain fonts #286

Merged

Conversation

fstrube
Copy link
Contributor

@fstrube fstrube commented Jul 7, 2022

Fixes #282.

This branch fixes a ReferenceError that gets thrown when parsing certain fonts, including the Playfair Display font that is in the fontkit repository. The error started happening when version 1.9.0 of fontkit was released.

     ReferenceError: Cannot access 'c3x' before initialization

The fix involved refactoring the parse() function to use block-scoping inside the switch case statements in that function. See the MDN docs on redeclarations.

@Pomax
Copy link
Contributor

Pomax commented Jul 8, 2022

It might make sense to preallocate all the variables outside the switch, as that would have been the effective behaviour when babel transpiled to var (due to hoisting) anyway.

…ch, so as to mimic the old behavior when Babel would transpile `let` to `var`. See foliojs#282.
@fstrube
Copy link
Contributor Author

fstrube commented Jul 12, 2022

@Pomax @blikblum I preallocated all the variables outside the switch as you suggested. The diff is much smaller, which is nice.

src/glyph/CFFGlyph.js Outdated Show resolved Hide resolved
src/glyph/CFFGlyph.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

I don't have PR merging powers, but happy to review.

@fstrube
Copy link
Contributor Author

fstrube commented Jul 13, 2022

Thanks for reviewing @Pomax! I made those two changes that you suggested.

There are also a few more instances of var declarations in that function.

Do you think it is worth changing them to let? Or should we leave them as is to avoid any unexpected behavior?

@Pomax
Copy link
Contributor

Pomax commented Jul 13, 2022

Any opportunity to move from scope-hoisting to block scoped vars is a good opportunity I'd say, but I'll let @devongovett decide whether that's something he would like to see in this PR.

@fstrube
Copy link
Contributor Author

fstrube commented Jul 19, 2022

Hi @devongovett, would you mind taking a look at this? It is causing most of our pdfkit projects that rely on OTF fonts to fail. I'm hoping we can get this merged in soon, then get foliojs/pdfkit#1367 updated and merged as well.

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Thanks!

@AlainGourves
Copy link

It is possible to make a new release with this PR ?
The latest version is 2.0.2, released on June 19 2022 before this PR, so the error

ReferenceError: Cannot access 'c3x' before initialization

still occurs (for example with the font PlayfairDisplay-Regular.otf found in /test/data/PlayfairDisplay).

Thanks in advance!

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.

ReferenceError: Cannot access 'c3x' before initialization
4 participants