Plugin system #142

6utt3rfly · 2021-05-02T06:27:22Z

(after #141)
Splitting out part of #123 to start a plugin system

move utilities from tests.js to test_utils.js (so they can be used by plugin tests)
added hooks class + tests
Added hooks to JSEP + tests for their arbitrary usage
Moved Ternary to a plugin. Default JSEP still includes ternary support.

- move utilities from tests.js to test_utils.js - added hooks class + tests - Added hooks to JSEP + tests for their arbitrary usage - Moved Ternary to a plugin. Default JSEP still includes ternary support.

6utt3rfly · 2021-05-02T06:30:18Z

src/hooks.js

+  run(name, env) {
+    this[name] = this[name] || [];
+    this[name].forEach(function (callback) {
+      callback.call(env && env.context ? env.context : env, env);


with JSEP, we kind of need access to the Jsep instance, so I think it should always be bound to this from the caller? I went back and forth on how to pass in local information, but since the plugins need to be able to manipulate the results - or provide new results, without returning a value, I still ended up with this.node and this.nodes being passed in and then read back out. It allows reassigning a value, or even clearing a parsed tree. Any better ideas?

The usual pattern is that the hook creation code just passes this instead of a custom env object.
Are there any local variables we want to be passing that are not part of the state on this?

Oh I see. Yeah, in that case the typical pattern is that you pass in {context: this, node, nodes} as the env. I don't think it's a good idea to have instance properties that are essentially local variables that don't make sense outside the execution context that created them. Are these properties more generally useful?

Thanks for the suggestion! I'll push a change that uses env.context and env.node. See if it's more what you had in mind?

6utt3rfly · 2021-05-02T06:31:27Z

src/jsep.js

-		const nodes = this.gobbleExpressions();
+		Jsep.hooks.run('before-all', this);
+		this.nodes = this.gobbleExpressions();
+		Jsep.hooks.run('after-all', this);


could potentially pass in the nodes[] array. Hooks wouldn't be able to reassign to it (but would be able to push/pop to manipulate the array)

You're passing in this, so you're passing that as well. Not sure what change you're proposing?

Edit: nvm, got it. See comment above.

I changed this to run with just env.node. So now all uses would just be accessing env.node.

6utt3rfly · 2021-05-02T06:32:33Z

src/jsep.js

 	 * @returns {?jsep.Expression}
 	 */
 	gobbleExpression() {
-		const test = this.gobbleBinaryExpression();
-
+		this.node = false;


I know this pattern was commented on in #123 - it's basically trying to set an initial value, then it can check if it was set by the hook (without having a return value from multiple potential hooks). Any other ideas or suggestions?

If you actually use an env object (with context: this) instead of this, you can just check for the presence of a given property. Not sure how generally useful node can be if read at random times and I can easily see bugs happening because some code forgot to reset it.

LeaVerou

As a general comment, we should have hooks and variables exposed to hooks be guided by the needs of actual plugins, not by the needs that we anticipate theoretical future plugins may have. Thanks to your fantastic work, we have a good starter set of plugins that can guide us in terms of what we need to do. Here, I see a lot of working around to cater to the use case of plugins modifying the current node before it's returned. Is this something the plugins we have actually require? (it may be -- I don't know).
Also, I think essentially you are using this.node to mean "return value of the current function" which is going to be quite confusing. Do we actually need to be able to override every possible return value?

LeaVerou · 2021-05-04T12:42:19Z

src/jsep.js

 	 * @returns {?jsep.Expression}
 	 */
 	gobbleExpression() {
-		const test = this.gobbleBinaryExpression();
-
+		this.node = false;


If you actually use an env object (with context: this) instead of this, you can just check for the presence of a given property. Not sure how generally useful node can be if read at random times and I can easily see bugs happening because some code forgot to reset it.

src/jsep.js

- switch from `this.node` to local method object `env.node` - make after-all hook use env.node instead of nodes - Update readme

LeaVerou · 2021-05-05T10:03:42Z

src/hooks.js

+   * @public
+   */
+  add(name, callback, first) {
+    if (typeof arguments[0] != 'string') {


Nitpick: spaces for indentation in this entire file (I assume it's the default setting in your editor, so it happened when you copy/pasted).

So sorry! I added .editorconfig and added the tab as a rule to eslint so it should be fixed :-)

LeaVerou · 2021-05-05T10:06:49Z

LGTM, I guess we should wait until the other PR is merged now.

add lint rule for tabs update all spaces to tabs to pass lint

# Conflicts: # rollup.config.js # src/jsep.js # test/tests.js

6utt3rfly · 2021-05-18T05:24:59Z

I merged in the main branch to update. I checked the performance of this PR versus the current and I'm a little concerned that this seems to make jsep significantly slower in parsing expressions (~35% slower, from my tests so far). There are ways to make it less readable for some minor gains (i.e. trading off setup time vs parsing time, possibly removing some hooks that aren't used just yet). Any thoughts?

- add Jsep.hooksAdd() shortcut to Jsep.hooks.add() - pre-compute the calls to all registered hooks, binding to this from the constructor. Wrap the object-handling of the node argument into the hooks and return the result. This changes `Jsep.hooks.run('name', { context: this, node: {} })` to `updatedNode = this['name'](node)'` - remove `before-all` and `after-all` hooks - updated tsd.d.ts (including spacing)

6utt3rfly · 2021-05-23T19:37:18Z

@LeaVerou - I made another update for performance optimization:
Current Main: 100%
Current release (before the recent PRs, switching to a class): 78.87%
Pre-optimization: 67.26%
Now: 84.04%

I can revert if you prefer, but I felt that the performance hit of the hook calls was worth the change (and it's easier to use within the code, after the constructor pre-computes the calls). I'm open to further feedback and thoughts if you have any :-)

LeaVerou · 2021-05-24T09:36:43Z

It would be good to see some actual timings instead of deciding solely on the basis of percentages. A 35% increase in running time can be ok if it takes execution time from 2ms to 3ms, but not ok if it takes it from 400ms to 600ms (numbers are random to make a point).

In a few words, what do you think helped improve performance in the latest commit? Is it adding the hooksAdd() method? Is it adding the hook functions on this? It might be a good idea to tease those apart if they are actually separate and see if it's one of the two that makes the biggest improvement.

It would be good to find a way to improve performance that doesn't involve repeating hook names all around the codebase. E.g. if we know we always want to bind to this, maybe we can remove the context parameter entirely and just provide this to the function as an explicit parameter (with optional extra arguments). Then perhaps we could make this optimization behind the scenes, transparently?

Overall, I would have liked to merge this PR at the state it was before the last commit, and iterate on the performance improvement vs maintainability tradeoff of the last commit in a separate PR.

6utt3rfly · 2021-05-24T16:44:37Z

The biggest performance bottleneck seemed to be from calling the hooks.run() method with the created { context: this, node: xx } and then using env.node everywhere. With multiple hook calls for each part of the expression parsing, it adds up. Changing to separate arguments for binding and the arg didn't have too much impact. You're right that it's probably negligible overall if it's only used to parse a single expression (versus an entire code base, for example)...

Would you be okay with a local method like this?

  runHook(name, node) {
    if (Jsep.hooks[name]) {
      const env = { context: this, node };
      Jsep.hooks.run(name, env);
      return env.node;
    }
    return node;
  }

it still has the if-statement for every run call, but then no setup in the constructor. It simplifies the callers and keeps performance close to the original. Or I can just back out my change (or create a new PR from the earlier commit)? The hooksAdd was just a convenience and for adding to the types file, but can be removed too, if you prefer.

LeaVerou · 2021-05-24T21:54:46Z

Would you be okay with a local method like this?

I think so, but is this and node the only state we ever pass to a hook?
Though if this is faster, we could at least use it on the hooks that it covers.
But if the hooks only ever need this and node we could just pass node as a parameter and bind to this with a custom hook implementation, which would save us from creating env each time, surely that will improve performance a bit.

Though if we're down to object creation and binding making a difference, I suspect the gains or losses are too marginal to matter, even with parsing multiple expressions repeatedly.

- Remove pre-compute optimization, but add local method this.runHook(name, node). This will check if there is a hook to run before calling hooks.run() in order to save time. Performance is around 89% of the code without hooks (but without the optimization was around 67% - mostly due to making the { context: this, node } object)

6utt3rfly · 2021-05-25T04:26:40Z

Hopefully okay now @LeaVerou ? Or if it needs anything else changed just let me know! :-)

6utt3rfly · 2021-06-05T02:55:49Z

Any further thoughts on this @LeaVerou ?

LeaVerou

Sorry, I thought we had merged this! Approved.

6utt3rfly added 2 commits May 1, 2021 23:18

Rename tests.js to jsep.test.js

3ffc3a1

Plugin System:

ce3134c

- move utilities from tests.js to test_utils.js - added hooks class + tests - Added hooks to JSEP + tests for their arbitrary usage - Moved Ternary to a plugin. Default JSEP still includes ternary support.

6utt3rfly commented May 2, 2021

View reviewed changes

LeaVerou reviewed May 4, 2021

View reviewed changes

Plugin update

3602893

- switch from `this.node` to local method object `env.node` - make after-all hook use env.node instead of nodes - Update readme

LeaVerou reviewed May 5, 2021

View reviewed changes

6utt3rfly added 3 commits May 5, 2021 16:38

Add .editorconfig to set tabs (not spaces)

7bbdd2d

add lint rule for tabs update all spaces to tabs to pass lint

Merge branch 'commonjs' into plugin-system

9e88f5f

# Conflicts: # rollup.config.js # src/jsep.js # test/tests.js

Merge branch 'master' into plugin-system

7fdd5dc

6utt3rfly requested a review from LeaVerou May 23, 2021 19:37

LeaVerou approved these changes Jun 5, 2021

View reviewed changes

6utt3rfly merged commit f987680 into master Jun 5, 2021

6utt3rfly deleted the plugin-system branch June 5, 2021 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin system #142

Plugin system #142

6utt3rfly commented May 2, 2021

6utt3rfly May 2, 2021

LeaVerou May 4, 2021

LeaVerou May 4, 2021 •

edited

6utt3rfly May 5, 2021

6utt3rfly May 2, 2021

LeaVerou May 4, 2021 •

edited

6utt3rfly May 5, 2021

6utt3rfly May 2, 2021

LeaVerou May 4, 2021

LeaVerou left a comment

LeaVerou May 4, 2021

LeaVerou May 5, 2021

6utt3rfly May 5, 2021

LeaVerou commented May 5, 2021

6utt3rfly commented May 18, 2021

6utt3rfly commented May 23, 2021

LeaVerou commented May 24, 2021

6utt3rfly commented May 24, 2021

LeaVerou commented May 24, 2021 •

edited

6utt3rfly commented May 25, 2021

6utt3rfly commented Jun 5, 2021

LeaVerou left a comment

Plugin system #142

Plugin system #142

Conversation

6utt3rfly commented May 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeaVerou May 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeaVerou May 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeaVerou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeaVerou commented May 5, 2021

6utt3rfly commented May 18, 2021

6utt3rfly commented May 23, 2021

LeaVerou commented May 24, 2021

6utt3rfly commented May 24, 2021

LeaVerou commented May 24, 2021 • edited

6utt3rfly commented May 25, 2021

6utt3rfly commented Jun 5, 2021

LeaVerou left a comment

Choose a reason for hiding this comment

LeaVerou May 4, 2021 •

edited

LeaVerou May 4, 2021 •

edited

LeaVerou commented May 24, 2021 •

edited