Initialize date adapter with chart options

[benmccann]

Opening for discussion purposes

I use Chart.js to plot a chart for each stock in my portfolio. Most are US stocks traded in the NYC timezone. However, some are UK stocks traded in the London timezone. With the current global date adapter I cannot set a different time zone for each stock chart. One idea I had for this was to make a chart option that is accessible to the date adapter and then pass the options into the adapter.

One thing that's a little messy is having to pass the adapter object around so much in the time scale. That's because the time scale has so many static methods. We could make these instance methods and cache the adapter on the scale which might make it a little cleaner.

Some tests are failing because they use a mock chart object which would need to be updated to define the new chart.adapters._date

Related to #5960
Closes #6045

[kurkle]

If option are require, I would take a key for adapter options (like scale.options.adapters.date), and pass that to format, parse, startOf and endOf, keeping the adapter stateless (and simple).

(edit, added more functions that would need options)

[benmccann]

I debated that as the other alternative. The tradeoff is that the options would need to be passed to every method requiring them, which would make calling the adapter methods harder. I also thought it might be difficult to envision every use case for every adapter. By passing the options into the adapter, they
are available to every adapter in every method. If you decide later that you want one adapter to define its own options for a certain method then that's really easy. But when you make it part of the method signature, then if one adapter needs options for a method then you have to change the interface for all adapters

[kurkle]

• There should be no onverhead in passing the options as argument vs accessing options from a instance variable. Both are references to actual options, unless cloned of course.
• options can be passed to every function to achieve same level of configurability as by instantiating
• There is the this that you have to use in the stateful case which would grow the minified adapter some bytes. In stateless, similar overhead probably comes from the calling end, fetching the reference to those options.
• Instantiating would consume some extra memory.
• The need to pass instance pointer around or move those helper functions in time.scale.js to the exported interface. Same problem with options, but they are already available in half of the cases.

stateful:

add: function(amount. unit) {
    return do_stuff(this.options.requiredOption)
}

stateless:

add: function(amount, unit, options) {
    return do_stuff(options.requiredOption);
}

Some of these points make no difference, rest promote stateless design.
The difference is still really small, any other points to consider?

[simonbrunel]

This PR was the way I initially implemented adapters but after lot of discussions with @kurkle, we decided to go the static way. In #5960, there is a note about adding support for adapter options: passing an extra options to all methods was how we envisioned that feature.

The difference is still really small, any other points to consider?

These adapter options should be defined and collected at the time scale level, not (only?) at the chart level. Having two time scales (in the same chart) with different set of adapter options seems a valid use case to me, so a unique adapter instance at the chart level is wrong, it should be at least created at the scale level.

An adapter should not be considered to target a specific feature (the time scale), we could decide to use it for anything that is date related. I would make the trailing options object optional so we can call these static methods easily from anywhere without adapter options.

That's why I'm still thinking we should keep adapters stateless.

[benmccann]

I agree both that we should collect options at the time scale level and that the options should be optional.

An adapter should not be considered to target a specific feature (the time scale)

I'm not sure if affects how we pass in the options. We need to have some options object defined that gets passed in. It's just a question of do we pass them in via the constructor or method.

My main concern about passing the options as parameters is that it makes the API harder to evolve. If we want to add a new optional parameter, does it go before or after the options? If the new parameter goes after the optional parameter then we're no longer following a consistent patter of putting them last. If it goes before, then we've broken backwards compatibility. Maybe we don't care at the moment since these are private, but I imagine at some point in the future we'll make them public and we'll have much less flexibility if the options are parameters.

I typically think of a stateful object as one that is keeping track of state. There is no changing state here however. One way to be sure there's no changing state would be to call Object.freeze on the options passed in to make them immutable. I'm not sure if there's some design principle we're trying to follow or a goal we're trying to achieve not already mentioned with regard to statefulness? Any difference in memory or network usage seems negligible and a premature optimization.

[benmccann]

There's not a huge difference between approaches. new adapter(options).parse(value) vs adapter.parse(value, undefined, options) both have all the same functionalities, so I'm happy to go with whatever is decided. @simonbrunel do you still prefer the stateless approach after reading the discussion above and seeing both PRs? If you prefer the stateless approach, I will close this one and we can go with #6045

[simonbrunel]

I'm really divided between both approaches:

#6016: I feel there is something wrong with creating multiple instances of an adapter. We will need to create one adapter per feature (currently, one adapter per time scale). I'm not worry about performance but it will make the state synchronization more complex than #6045 since we will need to update adapter options (or recreate new adapters) on every chart.update().

#6045: I like the simplicity of this stateless approach, one adapter for the whole app and no need to synchronize options. However, I have the same concern as @benmccann about how options are passed to each method and how it can impact evolution of the API. (Note: I don't think we should merge format and weekday in the adapter options, but that's another topic).

One idea to make #6045 more future proof would be to take the same direction as plugins, i.e. always pass two arguments to each adapter method: args with the functional arguments and options for the adapter specific options.

For example:

adapter: {
    parse: function(args, options) { ... }
    diff: function(args, options) { ... }
    //...
}

parse({value: '2019', format: 'AAAA'}, options)
diff({min: 1234, max: 4567, unit: 'hour'} ,options)

I think I like it because it makes the API stable in case we need to add or remove arguments (even more stable than the two current approaches). We discussed that point a bit with @kurle but it seems a bit overkill for this minimal adapter interface until v3. But I would maybe still consider it to avoid calls like adapter.parse(value, undefined, options) in favor of adapter.parse({value: value}, options).

[benmccann]

The args approach does solve the issues of API evolution and passing undefined args. It seems a bit verbose though. Also, one thing that I struggle with is how would you decide when to use that approach? You could theoretically change every function in the project to take an args object. I feel like there's something wrong that we're singling out this use case for it

Original code:

first = +adapter.startOf(first, weekday ? 'day' : minor);

With args:

first = +adapter.startOf({timestamp: first, unit: weekday ? 'day' : minor}, adapterOptions);

I think I probably prefer the original #6045 to having an args object

If we wanted an adapter per time scale then creating it during update would make sense with the approach in this PR. That doesn't seem like much of an issue to me though

[kurkle]

Some quick thoughts of args approach

Pros:

• increased readability
• extensibility (mutable)
• free order of arguments

Cons:

• instead of argument order, have to remember exact names
• bad for minimization
• interface is mutable

[simonbrunel]

Also, one thing that I struggle with is how would you decide when to use that approach?

We started to use this way for our plugin hooks for exactly the same ordering issues, but we also decided to go that way for the controller update and render methods, for again the same reasons. So I feel we tend to prefer this approach for our public API since it makes function interface stable when adding, removing or reordering arguments. I would definitely not apply this rule for our private code, except when the number of arguments become too high (e.g. > 4, which I think is an ESLint rule).

@benmccann the correct example is:

first = +adapter.startOf(first, weekday ? 'day' : minor, adapterOptions);
// ... vs ...
first = +adapter.startOf({value: first, unit: weekday ? 'day' : minor}, adapterOptions);

I think I probably prefer the original #6045 to having an args object

I'm not sure why: let's say tomorrow we decide to remove one argument other than options, then we will need to keep calling the method with undefined in place of the removed arguments (e.g. we can't remove / reorder b and c in foo: function(a, b, c, options) if we decide to get rid of them). Almost similar without the trailing options (this PR) but less probable.

Though, that was a suggestion, which I think I prefer if we go with #6045, but I'm still open to discuss the dynamic approach of that PR. Note that even if the API is private, we still need to not introduce breaking change for external adapters.

[simonbrunel]

@benmccann can you update this PR with adapters created in the time scale during update, and picking adapter options from the scale options (the same I guess as #6045). Also, adapters shouldn't define a new class but instead override the base class (helpers.extend(adapter.prototype, { ... }), so we can keep the abstract logic working.

Then it will be easier to compare both solutions with the same behavior.

[benmccann]

I pushed a new version of this PR that creates the adapter during update. I can fix overriding the base class if we decide to go with this approach, but left that for later

[simonbrunel]

Ok, let's iterate a bit more on this one which I think I tend to prefer.

[kurkle]

I'm starting to like this approach better

[kurkle]

Maybe add some documentation about where to specify adapter options, or should that be left to adapters?
My opinions is to point the location here and to consult adapter docs for available options.

[benmccann]

For now, I think it might be best to leave the documentation to the individual adapters because the built-in moment adapter doesn't specify any options and so might confuse people who are using that one to see options mentioned but then not find any. If you're using the Luxon adapter or another one then the options can be made fairly prominent in those docs

[kurkle]

For now, I think it might be best to leave the documentation to the individual adapters because the built-in moment adapter doesn't specify any options and so might confuse people who are using that one to see options mentioned but then not find any. If you're using the Luxon adapter or another one then the options can be made fairly prominent in those docs

I agree, but for other adapters, the location that is passed to the adapter is not up to the adapter, but core. So I was thinking something like: "If you are using an external adapter that supports or needs options, those are passed from scale.adapters.date"

[benmccann]

Ok. I added a line to the docs. Let me know what you think

[simonbrunel]

It looks great, thanks @benmccann for all the changes.

[A1vinSmith]

remove the extra options