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

Non-portable use of String method .includes #1093

Closed
ccorn opened this issue Jan 26, 2018 · 11 comments
Closed

Non-portable use of String method .includes #1093

ccorn opened this issue Jan 26, 2018 · 11 comments

Comments

@ccorn
Copy link
Contributor

ccorn commented Jan 26, 2018

KaTeX v0.9.0-beta chokes on \mathbb{C} (and \mathbb{R} so it's probably the macro), telling me "undefined not callable".
KaTeX v0.8.3 and v0.9.0-alpha2 render that without problems.
Found using katex.renderToString with Duktape.

@kevinbarabash
Copy link
Member

Hmm... \mathbb{C} works in the test page on master, but that uses katex.render. I wonder if there's a difference between that and katex.renderToString.

@kevinbarabash
Copy link
Member

I tried katex.renderToString("\\mathbb{C}") on master and that works as well.

@kevinbarabash
Copy link
Member

I checked v0.9.0-beta as well. What's Duktape? Could you post a more complete code example?

@ccorn
Copy link
Contributor Author

ccorn commented Jan 26, 2018

Hmm, when I try it with less intervening Ruby layers, it works. Sorry, that's not your bug, even though it manifests at the Javascript execution level. Have to track it down myself.

@ccorn ccorn closed this as completed Jan 26, 2018
@ccorn
Copy link
Contributor Author

ccorn commented Jan 27, 2018

Use of the string method includes seems to be the culprit. Using the polyfill from Mozilla's documentation makes the error go away.
Found by using Spidermonkey 1.8.5 instead of Duktape, which gives more detailed error messages: TypeError: fontFamily.includes is not a function
I still have not figured out why a bare call to katex.renderToString succeeds however.

@ccorn ccorn reopened this Jan 27, 2018
@ccorn
Copy link
Contributor Author

ccorn commented Jan 27, 2018

I'd propose to use .indexOf(...) !== -1 instead, for compatibility. Or including the polyfill.

@kevinbarabash
Copy link
Member

Nice find!

@kevinbarabash
Copy link
Member

We'll definitely want to fix this before the next release.

@ccorn ccorn changed the title "undefined not callable" for \mathbb{C} Non-portable use of String method .includes Jan 27, 2018
ccorn referenced this issue Jan 27, 2018
* Adding support for SansSerif-Bold

* Updating to include SansSerif Italic.

* WIP

* Working text stacking

* More robust screenshot.

* Don't want to break users :)

* Updating per PR comments.

* Fixing Unicode and updating snapshots.

* Adding suggested tests.

* Opting to use old method for unit testing.

* Adding TODO
@kevinbarabash
Copy link
Member

@ccorn if you have time, would definitely accept a PR to fix this.

@ccorn
Copy link
Contributor Author

ccorn commented Jan 27, 2018

I'd like to do that, but I am not a Javascript expert, and I am not familiar with the build system around KaTeX. I can fork the repo, do a minimal .indexOf replacement, but then I would not know how to test that change. I could launch a PR and hope that some CI kicks in and that everything turns out well, but if not, then anyone familiar with KaTeX development should better take over. Also, compatibility issues such as this one are bound to happen again, and the preferred strategy might be different from fixing the instances as they are discovered.

@ccorn
Copy link
Contributor Author

ccorn commented Jan 27, 2018

Hmm, there is a Makefile. Known territory. OK, I'll ssh into a server that has Node available, and then try and clone and mess around a bit.
Update: Oh wow, that was easy. A git clone, a git submodule update --init --recursive, and a make seem to do what I need. Great. I'm on my way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants