Use a Map instead of compiled switch-case for keywords #173
Conversation
Otherwise changes to this method were not benchmarked.
…form This should be more performant in modern browsers and avoids the security issues of using the Function method with user input. This makes moo.js compatible with Content-Security-Policy values that block unsafe-eval. See #141
|
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 But I do agree it's a bit silly. What do you think @nathan? |
|
As @tjvr observes, there's essentially no overhead for falling back to |
The overhead of this try-catch applies only once at start-up when loading the keywords. I have tested that the fallback works in older browsers, and that there is no performance impact of the try-catch for modern browsers.
|
Sure. I have added a fallback to using |
moo.js
Outdated
| // Use a JavaScript Map to map keywords to their corresponding token type | ||
| // unless Map is unsupported, then fall back to using an Object: | ||
| var reverseMap, reverseMapGet, reverseMapSet; | ||
| try { |
There was a problem hiding this comment.
Why not just if (typeof Map !== 'undefined')?
moo.js
Outdated
| var reverseMap, reverseMapGet, reverseMapSet; | ||
| try { | ||
| reverseMap = new Map | ||
| reverseMapGet = function(k) { |
There was a problem hiding this comment.
The helper functions reverseMapGet and reverseMapSet make your code longer and more complicated than using an if/else on typeof Map in the appropriate places. Maybe call it var isMap = typeof Map !== 'undefined' and ternary on that.
|
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. |
moo.js
Outdated
| if (typeof keyword !== 'string') { | ||
| throw new Error("keyword must be string (in keyword '" + tokenType + "')") | ||
| } | ||
| reverseMap[keyword] = tokenType | ||
| isMap ? reverseMap.set(keyword, tokenType) : reverseMap[keyword] = tokenType |
There was a problem hiding this comment.
Use an if statement here. You're not doing anything with result of the expression.
As noted in #141, using
Functioncauses issues for websites using a Content-Security-Policy that blocksunsafe-evaland 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.keywordssince 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.