Option resolution with proxies #8260

kurkle · 2020-12-31T15:50:54Z

As #8008 gets overly complicated, I thought of an (yet another) alternative way for resolving the options.

Using proxies, and split in 2 parts.

_resolver resolves the raw values on property access (and caches that).
_withContext applies an context on top of that.

One really nice advantage of this method is, that we can pass the _withContext object to the scriptable function, so all other values at that context are available. See the tests for example.

Hoping for comments on this :)

etimberg · 2020-12-31T15:57:23Z

src/helpers/helpers.options.js

 	}
 }
+
+const notIndexable = ['borderDash', 'fill', 'events'];


Would there ever be a need for plugins to mark fields as not indexable?

Probably. Extensions too. So this needs to be designed so it can be expanded (on register I'd imagine)

Even if not hardcoded (which should definitely not be the case), determining if an option is indexable (or scriptable) should not rely on the option name. For example, a plugin could use one of these names as option (e.g. fill) while still expecting it to be indexable/scriptable. This information should be pass to the resolver (as it is with the existing resolve() method).

etimberg · 2020-12-31T16:01:08Z

src/helpers/helpers.options.js

+	const cache = Object.create(null);
+	return new Proxy(cache, {
+		get(target, prop) {
+			let value = Reflect.get(target, prop);


My first thought on this was performance and needing to look up the loop over and over again. Reflect is a nice solution to this!

src/helpers/helpers.options.js

etimberg · 2020-12-31T16:06:42Z

test/specs/helpers.options.tests.js

+			const defaults = {
+				color: 'red',
+				backgroundColor: 'green',
+				hoverColor: (ctx, options) => getHoverColor(options.color)


This is pretty powerful and would enable pulling the hover settings out into defaults

Indeed, but it will need to support throwing an error in case of cyclic dependency.

I think all platforms throw at some depth, which is quickly reached on a cyclic dependency.

It would be interesting to test this case on a few browsers. It should not freeze the browser but also be easy to debug such case. Ideally, we should throw an error with the dependency loop explained.

// should throw at the "third" recursion when trying to resolve either foo, bar or xyz const options = { foo(ctx, opts) => opts.bar, bar(ctx, opts) => opts.xyx, xyz(ctx, opts) => opts.foo, }

This is how it goes with the POC:
https://codepen.io/kurkle/pen/LYRdrrQ

It's indeed fast to fail, though hard to debug but maybe that's because it's a codepen.

kurkle · 2020-12-31T21:21:13Z

made some progress with performance testing:

master:

100 runs done in 5477ms. Average: 54ms, min: 32ms, max: 173ms, variation: 141ms. usedHeap: 46289.1kB

resolver:

100 runs done in 5977ms. Average: 59ms, min: 37ms, max: 218ms, variation: 181ms. usedHeap: 32324.2kB

so not quite there, but not bad either. most test still fail, only go the line controller functioning.

kurkle · 2021-01-03T08:39:19Z

Made some more progress, got tests passing. This branch is POC: https://github.com/chartjs/Chart.js/compare/master...kurkle:resolver-wip?expand=1

Things achieved:

all top level options are scriptable. (top level for chart, dataset, scale and plugin options)
hover color resolution in defaults
the optimal case (uPlot bench) has similar performance with master

Couple of issues encountered:

Sub objects need to be have their own _resolver to work correctly. The sub-objects are currently not scriptable/indexable.
jasmine.objectContaining does a lot of checks to the object and that did not work with (current) Proxy implementation
- need to either implement the things jasmine checks or use a custom matcher (thats what I did in the POC)
need to mark options as not scriptable somehow. event handlers for example.

simonbrunel · 2021-01-03T10:47:08Z

I think I like to general idea of this PR (if I understand it correctly, which I'm still unsure), but there is a few things that should not be implicit for the caller of this method (i.e. we should not guess if an option is supposed to be scriptable / indexable). However, I would not expect this logic to replace the simple resolve() one but be an extra high level API.

Some additional thoughts: few years ago, I was thinking to introduce option descriptors that would describe the way options are shaped and resolved. I don't remember exactly of the syntax, but conceptually it was something like this:

const descriptors = {
  color: {
    indexable: true,
    scriptable: true,
    default: '#123456',
    values: [
      (chart, ctx) => chart.data.datasets[ctx.datasetIndex].color,
      'options',
      { source: 'options', key: 'baseColor' },
      'defaults'
    ],
  },
  borderDash: {
    indexable: false,
    scriptable: true,
    default: false,
    values: [
      (chart, ctx) => chart.data.datasets[ctx.datasetIndex].borderDash,
      'defaults'
    ]
  },
  events: {
    indexable: false,
    scriptable: false,
    default: {},
    values: [
      'options',
      'defaults'
    ]
  }
}

const options = helpers.options.describe(descriptors, {
  sources: {
    'defaults': Chart.defaults,
    'options': chart.options,
  }
});

/*
const resolvedColor = options.color(context, index);
const resolvedBorderDash = options.borderDash(context);
const resolvedEvents = options.events;
*/

Having descriptors also allows to pre-optimize option resolution by creating accessors (proxies?) that skip some checks when not indexable, scriptable or if a source is null.

simonbrunel · 2021-01-03T11:17:50Z

Maybe it's a bit out of the scope of this PR but something else that could simplify the option resolution logic would be to fully drop support for indexable options in v3. IMO, they are cumbersome, limited and not anymore useful since scriptable options have been introduced:

const colors: ['blue', 'red', 'yellow'];

const options = {
   colors: colors,   // don't support index > 2,

   // eq.

   colors: ({ index }) => colors[index],

   // or better:

   colors: ({ index }) => colors[index] || 'grey',

   // or a different use case:

   colors: ({ index }) => colors[index % 3]
}

But additionally, we could add an indexable helper that would return a scriptable functor:

const options = {
  color: helpers.indexable(colors),

  // or

  color: helpers.indexable(colors, {
    key: 'index' | 'dataIndex' | 'datasetIndex' | '...',
    loop: false | 'repeat' | 'alternate',
    reverse: true,
    default: 'grey'
  }),
}

So controllers don't have to care anymore about how to "interpret" this option array.

benmccann · 2021-01-03T13:59:10Z

I'd be supportive of dropping support for indexable options (probably first as a separate PR to keep this one from getting too large).

I didn't look at #8008 too closely so I'm curious what it was that you considered too complicated with it

etimberg · 2021-01-03T15:40:39Z

I think dropping indexable options might be quite disruptive so I'd want to avoid that. The descriptors idea looks promising in that it would be allow simplifying the lookup code (plus it would address the concerns above which options are indexable) and we could allow plugins to provide descriptors. I think for legacy reasons we'd need to support no descriptors, but that could follow the existing rules.

kurkle · 2021-01-03T19:53:25Z

Thanks all, good feedback!

Indexable

I agree with @etimberg it would be quite disruptive to stop supporting that. I had thought of it, because it can not be used on all options - but now that I tested making everything scriptable, I noticed we have quite a few callbacks (that can not be scriptable). So that same/similar issue remain even if indexable was removed. The resolution would be simpler without indexable options, but not by much.

Descriptors

Interesting idea, combine all the configurable things about an option to single entity. As a DBA I'd say its almost like flat vs normalized. Descriptors being flat and defaults, scopes, indexability and scriptability separated being normalized. In a DB that means more space used. Performance depends on many things.

I'm not sure how we would describe the different resolution paths we currently have, depending on the scope a option is resolved to. Eg, borderColor is resolved differently for datasetElement and dataElement. Yet again differently for tick.

It would not solve "determining if an option is indexable (or scriptable) should not rely on the option name." (I'm not sure that can be solved).

We would need to register the descriptors (instead of the plain defaults we not register). Actually that is just the thing we really need. Registering the indexability / scriptability of options with defaults. Those can be either flat or separate, and we can even flatten or normalize those at registration, depending on what turns out to be the best solution.

Some of the things I'm trying to consider/address in this pr

defaults are mixed with options currently (merged), and after that we can not resolve anything using a different path correctly, so that every level of defaults is considered after all the options user provided.
after merge, chart.options contains a lot of excess stuff, that has no effect to anything
a lot of the code is about option resolution, try to normalize that (and make everything work consistently)
have the resolution built in, consuming an option is just reading the property. the consumer does not need to care if it was scripted, indexed or whatever. this would be really useful for plugins, you'd get scriptable options out of the box without a line of code.
normalize the option resolution path, so everything is resolved consistently
avoid the maintenance work when introducing new options or making old options scriptable all over the place one by one
extensions and their controllers should be able to use the same resolution logic core has, without copying and maintaining the copy of the code
there should be the user provided options object available somewhere. maybe chart.config.options should be that. chart.options could be the resolved/resolving options. likewise dataset should be the thing user provided. controller.options would be the resolved/resolving options.
not needing to list what options everything uses, resolve it when used.

8008 complexity

I realized we'd need to list every single option that is usable in each scope. Also really hard to get working after more than 100 commits to master after initial PR (not quite sure why).

simonbrunel · 2021-01-03T20:42:32Z

I think dropping indexable options might be quite disruptive so I'd want to avoid that

I'm curious to know if indexable options are used a lot and how. I'm fine with keeping support for indexable options as long as we don't introduce extra resolve option to support looping in the array. For example, color: colors will never resolve to: 0: 'blue', 1: 'red', 2: 'yellow', 3: 'blue', 4: 'red', 5: 'yellow'... if colors is ['blue', 'red', 'yellow'] (it should always return undefined for index > 2). I think this looping feature has already been requested, but it should be done using scriptable options. That's why I think we should not promote indexable options anymore, it's too limited.

simonbrunel · 2021-01-03T20:49:04Z

It would not solve "determining if an option is indexable (or scriptable) should not rely on the option name." (I'm not sure that can be solved).

It would because a descriptor describes a specific option. A fill option in the line controller is different of a fill option in a plugin. These are two different descriptors, used at different locations (e.g. one not indexable in the line controller and one indexable in the plugin). With the current PR implementation, all options named fill are considered not indexable.

A set of descriptors is indeed flat but there is as many sets as there is locations/scopes where we need to read options (plugins, controllers, elements, core, etc.). For example, I would generate one set of descriptors for the datalabels plugin. In my 2017 POC, for the line controller, there was a set of descriptors for the dataset elements and another one for the data elements.

Descriptors (and resolved options) are internal and should not be exposed to the public API.

kurkle · 2021-01-03T20:52:29Z

I think this looping feature has already been requested, but it should be done using scriptable options. That's why I think we should not promote indexable options anymore, it's too limited.

#7442

kurkle · 2021-01-03T21:03:28Z

It would not solve "determining if an option is indexable (or scriptable) should not rely on the option name." (I'm not sure that can be solved).

It would because a descriptor describes a specific option. A fill option in the line controller is different of a fill option in a plugin. These are two different descriptors, used at different locations (e.g. one not indexable in the line controller and one indexable in the plugin). With the current PR implementation, all options named fill are considered not indexable.

Agreed, the way its currently in this PR is not the way to go. (Or could be expect for the implementation of indexable helper and its parameters)

A set of descriptors is indeed flat but there is as many sets as there is locations/scopes where we need to read options (plugins, controllers, elements, core, etc.). For example, I would generate one set of descriptors for the datalabels plugin. In my 2017 POC, for the line controller, there was a set of descriptors for the dataset elements and another one for the data elements.

Descriptors (and resolved options) are internal and should not be exposed to the public API.

Wouldn't the descriptors (or the spec of a descriptor) be public API so extension/plugins can provide those?

simonbrunel · 2021-01-03T21:15:53Z

#7442

Ok, that's now the default behavior which is probably better than resolving to undefined. I would have close #4807 by suggesting to use a scriptable option since #7442 implements a specific use case. Here are some other ways to "loop" in that array if [a, b, c]:

alternate: a, b, c, b, a, b, c ...
fill: a, b, c, c, c, c, ...

I'm sure users will ask for them and my point is that we should not introduce new options to support these extra use cases. Instead, we should ask users to rely on scriptable options and maybe provide an helpers.indexable if it make things easier.

Wouldn't the descriptors (or the spec of a descriptor) be public API so extension/plugins can provide those?

I'm not sure I understand what you mean by "provide those". Descriptors are internal logic to describe how to resolve an option "required by a specific piece of logic". There is no dependency between descriptors, so I'm not sure why they would be public.

kurkle · 2021-02-05T13:28:27Z

Closing in favor of #8374

kurkle added 2 commits December 31, 2020 17:29

Option resolution with proxies

8b1dc59

re-scope the test constants

510dd1e

etimberg reviewed Dec 31, 2020

View reviewed changes

kurkle requested a review from simonbrunel January 1, 2021 18:34

Add recursion detection

8399f44

kurkle mentioned this pull request Feb 3, 2021

Option resolution with proxies #8374

Merged

2 tasks

kurkle closed this Feb 5, 2021

kurkle deleted the resolver branch March 10, 2021 06:49

Option resolution with proxies #8260

Option resolution with proxies #8260

Uh oh!

Conversation

kurkle commented Dec 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurkle commented Dec 31, 2020

Uh oh!

kurkle commented Jan 3, 2021

Uh oh!

simonbrunel commented Jan 3, 2021

Uh oh!

simonbrunel commented Jan 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benmccann commented Jan 3, 2021

Uh oh!

etimberg commented Jan 3, 2021

Uh oh!

kurkle commented Jan 3, 2021

Indexable

Descriptors

Some of the things I'm trying to consider/address in this pr

8008 complexity

Uh oh!

simonbrunel commented Jan 3, 2021

Uh oh!

simonbrunel commented Jan 3, 2021

Uh oh!

kurkle commented Jan 3, 2021

Uh oh!

kurkle commented Jan 3, 2021

Uh oh!

simonbrunel commented Jan 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurkle commented Feb 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

simonbrunel commented Jan 3, 2021 •

edited

Loading

simonbrunel commented Jan 3, 2021 •

edited

Loading