Closed 6utt3rfly closed 3 years ago
@LeaVerou - I tried testing with all plugins (including those not yet merged) and came across some intermittent difficulties with failing tests because jsep plugins and hooks were inconsistent between tests. Now it's fine with these changes, but I'm starting to rethink the earlier idea to have a JsepConfig class. What do you think?
Are there any other interface changes that should be made? Would you want to remove the addUnaryOpts
, addBinaryOps
, and other similar functions and just use the properties directly (although that would be a breaking change)?
Thanks, will try to review later this week.
Would you want to remove the
addUnaryOpts
,addBinaryOps
, and other similar functions and just use the properties directly (although that would be a breaking change)?
I generally avoid making breaking changes without deprecating first. If we decide we want to remove them, we should start printing a deprecation warning when they are used, and remove in the next version.
Now the question is, do we want to remove them? I'm fine with that, but would like to hear from you and @EricSmekens .
I also noticed we're still using a pre-release version 0.x.x ... but I still try to avoid breaking changes too, especially with such a large user base. I would propose bumping to v1.0.0 regardless whether the next release is breaking or not.
I'm somewhat indifferent on removing the accessor methods, but I noticed it was brought up in several comments. I know there are views on both sides, generally. In support of using methods, is that it maintains encapsulation and allows for checks and changing the data structure with non-breaking changes (i.e. from an object to a Set). In removing them, it can keep the code smaller and simpler. Longer term, it's probably simpler to keep them, or perhaps they could be condensed into a single config method if preferred (i.e. addOperators({ unary: ..., binary: ... })
?
I also noticed we're still using a pre-release version 0.x.x ... but I still try to avoid breaking changes too, especially with such a large user base. I would propose bumping to v1.0.0 regardless whether the next release is breaking or not.
I'm fine with that. JSEP has been on 0.x.y for 9 years, maybe it's about time, lol.
I'm somewhat indifferent on removing the accessor methods, but I noticed it was brought up in several comments. I know there are views on both sides, generally. In support of using methods, is that it maintains encapsulation and allows for checks and changing the data structure with non-breaking changes (i.e. from an object to a Set). In removing them, it can keep the code smaller and simpler. Longer term, it's probably simpler to keep them, or perhaps they could be condensed into a single config method if preferred (i.e.
addOperators({ unary: ..., binary: ... })
?
Not sure the benefit of condensing to a single method is worth the backwards incompatibility. I'd say let's just keep the existing methods for now, we can always remove them later. The data structures are available directly as well though, right?
The data structures are available directly as well though, right?
Yes they can be directly accessed. I also added the data properties into the ts file in this PR.
I 100% agree on making this 1.0.0.
Version wise, we could decide to drop the addBinaryOps
etc.
But as LeaVerou states, I think it doesn't hurt for now, and it will make the decision for all current users to update to this version a lot easier. Let's see what kind of feedback we get on this, we can always decide to drop them in a future 2.0.0.
Oops - I mean to push that to a feature branch for a PR! I'll clean that up shortly...
Merged into plugin-system
(without the accidental regex plugin commit)
jsep.hooksAdd
and just stick withjsep.hooks.add
. This allows jsep to be reset easier (i.e. between unit tests) without having to re-bind to jsepresetJsepHooks
test utility =>resetJsepDefaults
(which currently includes jsepTernary plugin)