search

no-context / moo

Optimised tokenizer/lexer generator! 🐄 Uses /y for performance. Moo.
BSD 3-Clause "New" or "Revised" License
833 stars 66 forks source link

Use a Map instead of compiled switch-case for keywords #173

Closed jsharkey13 closed 2 years ago

jsharkey13 commented 2 years ago

As noted in #141, using Function causes issues for websites using a Content-Security-Policy that blocks unsafe-eval and it also appears that this is no longer the fastest approach.

This PR moves to using a Map, which caniuse data suggests is supported in all major browsers released since 2015. I don't think the overhead of falling back to object attribute lookup is worthwhile unless you specifically support an ancient browser you know to be incompatible. I also altered the benchmark test to use moo.keywords since it was not doing so (although the difference to this benchmark is inside the noise when running in Node for me).

We're going to move to this Map approach in our project that uses moo.js, so we will have some real-world verification all works as expected in a few weeks if you want to wait for that.

Resolves #141.

tjvr commented 2 years ago

I would slightly prefer continuing to support ancient browsers just in case someone is still using this on IE10 for some reason 😅 I imagine we can avoid the overhead of this by checking once at startup and swapping out how moo.keywords is defined.

But I do agree it's a bit silly. What do you think @nathan?

nathan commented 2 years ago

As @tjvr observes, there's essentially no overhead for falling back to Object.create, and it doesn't complicate the implementation much. Dropping support for environments we currently support is arguably semver-major, so I think it makes sense to just keep supporting them.

jsharkey13 commented 2 years ago

Sure. I have added a fallback to using Object.create(null) if necessary, and tested that the method falls back and continues to work in Internet Explorer 10. (I didn't test the whole library, just the keywordsTransform function).

jsharkey13 commented 2 years ago

I think this is less clear than the helper functions; but I have again tested that it performs the same and still works in IE10, so hopefully this can progress now.