-
Notifications
You must be signed in to change notification settings - Fork 133
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
Convert JSEP to ES6 class, while maintaining backward compatibility #140
Conversation
Had a quick look through, thank you for this work on it! |
Hey, so sorry for being absent lately, I was extremely busy with moving. Now settling in slowly, and should be able to take a look in the next couple of days. |
From a very cursory look, a lot of this looks like what I had in mind, but I'm not sure about the added complexity of having three classes, I was envisioning it as a single |
When I was trying to write some plugins with the existing code, there were some constants that I had to duplicate because they were not accessible to the plugins in other files. I think it would be useful to have access to them from plugins, but I agree they could be an Object, or placed directly onto the class. I debated freezing it, but then thought it might be useful to allow to add to it (or possibly redefine, but I don't know a use-case for that). For having separate config/parsing classes, I was mostly just thinking of separation of responsibilities. The parsing class makes sense to construct and destroy with each parsing request (similar to existing behavior). It would also allow passing that class instance into plugins. I don't know if there are cases where different configurations of parsing might be useful to run concurrently, but it also seemed like an easy thing to support with that separation. To me, it felt wrong to have methods to add/remove operators that would affect static variables on the class (or hidden variables in the file that plugins would then not have access to) -- and I didn't want to have to copy them or do extra work in the constructor of every parser. Just my humble opinions though :-) |
Agreed that constants need to be available to plugins, and also agreed that the object shouldn't be frozen, so it can be extended. Your reasoning about the config class makes sense. I think I like the idea. |
Sounds good - do you prefer static getters, or assigning the constants as properties after declaring the class? |
I think simple properties are fine. |
src/jsep.js
Outdated
this.expr = expr; | ||
this.config = config; | ||
this.index = 0; | ||
this.length = expr.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be a getter? Though that might hinder performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ran a performance benchmark and there doesn't seem to be any performance difference between saving that local variable versus using this.expr.length
. A getter is about 60% slower though. Mostly I just kept it to try not to change the existing code too much
@LeaVerou - I pushed my changes to switch to a single class with static methods and fields. I also added a getter for Revised sizes:
|
I thought we'd still use a separate class for the config? Anyway, I'm fine either way, probably a good idea to start simple and take it from there. |
* @returns {Jsep} | ||
*/ | ||
static addIdentifierChar(char) { | ||
Jsep.additional_identifier_chars.add(char); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this function only exist for backwards compat? Because if additional_identifier_chars
is exposed, people can just add characters to it directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, although it was only just added recently, so could possibly be removed with minimal impact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
*/ | ||
static addBinaryOp(op_name, precedence) { | ||
Jsep.max_binop_len = Math.max(op_name.length, Jsep.max_binop_len); | ||
Jsep.binary_ops[op_name] = precedence; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not for this PR, but I think eventually we may want to move towards a data structure that just works, vs having to use special-purpose mutation methods. E.g. we could make binary_ops
an instance of a BinaryOpMap
which would inherit from Map
but intercept its methods to update the max length accordingly. Then people can just modify it like a regular map. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree. It makes sense to encapsulate and would allow reuse with the unaryOp.
src/jsep.js
Outdated
* @param {string} message | ||
*/ | ||
throwError(message, index = this.index) { | ||
Jsep.throwError(message, index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through the code and did not find a single usage of throwError()
with an index other than this.index
. Should we have this parameter at all? Also, is there a reason to have both a static and an instance method to throw errors? The static method does not appear to be used anywhere except internally. Is it used in plugins perhaps?
In general, I think it's good practice to start with a smaller API and grow as needed, than a larger one because certain needs are anticipated that may never arise. But that might be a philosophical difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
src/jsep.js
Outdated
* @returns {boolean} | ||
*/ | ||
isIdentifierPart(ch) { | ||
return this.isIdentifierStart(ch) || Jsep.isDecimalDigit(ch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a static method, it does not depend on this
in any way.
src/jsep.js
Outdated
* @returns {number} | ||
*/ | ||
charCodeAt(index = this.index) { | ||
return this.expr.charCodeAt(index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, adding methods that just wrap the same function calls on this.expr
seems like overabstraction. Especially since we already have convenience getters when what we want is this.expr.charCodeAt(this.index)
so this is literally just a wrapper for the function call.
Same applies to the charAt()
wrapper above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree - changed
src/jsep.js
Outdated
// top-level parser (but can be reused within as well) | ||
let gobbleExpressions = function(untilICode) { | ||
/** | ||
* Top-level method to parse all expressions and returns compound or singl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment seems truncated?
src/jsep.js
Outdated
* @param {jsep.Expression} right | ||
* @returns {jsep.BinaryExpression} | ||
*/ | ||
static createBinaryExpression(operator, left, right) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be fine inlined and would make the code more clear. It's only used twice and the function doesn't do much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
- make methods static where possible - throwError no longer static - inline createBinaryExpression method - rename currentChar/code to just char/code. Remove method, leave just getters for current index
*/ | ||
static addBinaryOp(op_name, precedence) { | ||
Jsep.max_binop_len = Math.max(op_name.length, Jsep.max_binop_len); | ||
Jsep.binary_ops[op_name] = precedence; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree. It makes sense to encapsulate and would allow reuse with the unaryOp.
* @returns {Jsep} | ||
*/ | ||
static addIdentifierChar(char) { | ||
Jsep.additional_identifier_chars.add(char); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, although it was only just added recently, so could possibly be removed with minimal impact?
src/jsep.js
Outdated
* @param {jsep.Expression} right | ||
* @returns {jsep.BinaryExpression} | ||
*/ | ||
static createBinaryExpression(operator, left, right) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
src/jsep.js
Outdated
* @returns {number} | ||
*/ | ||
charCodeAt(index = this.index) { | ||
return this.expr.charCodeAt(index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree - changed
src/jsep.js
Outdated
* @param {string} message | ||
*/ | ||
throwError(message, index = this.index) { | ||
Jsep.throwError(message, index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
} | ||
|
||
// Backward Compatibility (before adding the static fields): | ||
const jsep = expr => (new Jsep(expr)).parse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figure this can be Jsep.parse
or expr => (new Jsep(expr)).parse()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's Jsep.parse
then the functions we're adding to it afterwards would be added to Jsep.parse
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could however be expr => Jsep.parse(expr)
// Node Types | ||
// ---------- | ||
// This is the full set of types that any JSEP node can be. | ||
// Store them here to save space when minified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed this comment. If the only purpose of these constants is to "save space when minified", then we should get rid of them, per #131. What do you think?
I'm only referring to the expression type constants. The character code ones are obviously useful.
Not sure e.g. MEMBER_EXP
is easier to remember than "MemberExpression"
. The only benefit I see is that perhaps we can change the type without having to change it everywhere in the code, but are we ever going to do that? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm indifferent... there are some of the defines that are used multiple times, and others only once. For consistency it's kind of nice to have them here. And I think it's nicer to use a constant/enum rather than needing the exact string in several places.
const jsep = Jsep.parse; | ||
const staticMethods = Object.getOwnPropertyNames(Jsep); | ||
staticMethods | ||
.slice(staticMethods.indexOf('version'), staticMethods.indexOf('removeAllLiterals') + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which properties are we trying to avoid here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full list it returns:
0: "length"
1: "prototype"
2: "version"
3: "toString"
4: "addUnaryOp"
5: "addBinaryOp"
6: "addIdentifierChar"
7: "addLiteral"
8: "removeUnaryOp"
9: "removeAllUnaryOps"
10: "removeIdentifierChar"
11: "removeBinaryOp"
12: "removeAllBinaryOps"
13: "removeLiteral"
14: "removeAllLiterals"
15: "parse"
16: "getMaxKeyLen"
17: "isDecimalDigit"
18: "binaryPrecedence"
19: "isIdentifierStart"
20: "isIdentifierPart"
21: "name"
(and would add all the subsequent ones too if I moved this to the end of the file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! See 2 minor comments (more like questions).
Merged! Thanks again! @EricSmekens can we add @6utt3rfly as a collaborator? I think it's very well deserved. |
Yes, I fully agree! Will do that in a few minutes! |
Follow-up from #123, and related to #137 and #130
Converts JSEP to use ES6 classes (but roll-up builds ESM, IIFE, and CJS):
Constants
- just the string/numerical constantsJsepConfig
- all the current functions on jsepJsep
- the parser. Constructed withexpr
and parsed withparse()
Maintained backward compatibility by exporting a default function that uses those classes. Tests are unchanged to ensure consistency.
Performance comparison:
Before PR: 4303.82 op/s +/- 2.09% [5.74% slower]
This PR: 4566 ops/s +/- 2.99%
(from jsbench.me with both versions in the setup. Ran all of the current test cases in both versions)
Size comparison:
Hopefully OK that I took a stab at this, @LeaVerou ? I'm happy to change it or whatever :-)