Skip to content
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

Add alternative signatures for Model#start and Model#on that don't use var-args #263

Merged
merged 5 commits into from
Jul 11, 2019

Conversation

ericyhwang
Copy link
Contributor

@ericyhwang ericyhwang commented Jul 9, 2019

The var-args in the middle of parameter lists for Model#start and Model#on make it difficult to write TypeScript type definitions for them.

These backwards-compatible changes add alternative signatures that don't use var-args.

  1. Model#start now supports a new array format for input paths:
model.start(outPath, inPath1, inPath2, ..., fn);  // Old
model.start(outPath, [inPath1, inPath2, ...], fn); // New
  1. Model#on has a new {useEventObjects: boolean} option. If true, it calls the listener with an Event object and the wildcard-captured segments as an array:
// Old
model.on('change', 'foo.**', (captures..., value, previous, passed) => {
});
// New
model.on('change', 'foo.**', {useEventObjects: true}, (changeEvent, captures) => {
  const {type, value, previous, passed} = changeEvent;
});

Once this is published, I'll update derby-site with documentation on the new signatures and event objects.

…e var-args

The var-args in the middle of parameter lists for Model#start and Model#on make it difficult to write TypeScript type definitions for them.

These backwards-compatible changes add alternative signatures that don't use var-args.

1) Model#start now supports a new array format for input paths:

  model.start(outPath, inPath1, inPath2, ..., fn);  // Old
  model.start(outPath, [inPath1, inPath2, ...], fn); // New

2) Model#on now checks if the path pattern is a pre-split array, and if so, it calls the listener with a ____Event instance and the wildcard-captured segments as an array:

  model.on('change', 'foo.**', (captures..., value, previous, passed) => {});  // Old
  model.on('change', ['foo', '**'], (changeEvent, captures) => {});  // New

A ChangeEvent would look like {value, previous, passed}.
Copy link
Contributor

@nateps nateps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments for changes

lib/Model/events.js Outdated Show resolved Hide resolved
lib/Model/events.js Outdated Show resolved Hide resolved
lib/Model/events.js Show resolved Hide resolved
lib/Model/events.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ericyhwang ericyhwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made changes based on additional discussion in person:

  1. Added a {useEventObjects: true} option to Model#on, which causes the callback to be invoked as cb(event, captures) instead of the now-legacy var-args cb(captures..., [eventType], eventArgs..., passed). This is more explicit than argument-detection, and it enables the use-case you brought up of passing in a scoped model.
  2. Split old code in modelEventListener out into a separate modelEventListenerLegacy function.
  3. Because we no longer need argument-detection, I've reverted the change to take the pattern as an array of path segments. Even though we still want to move that direction longer term, it's weird to have Model#on be the only thing in the public API that accepts path segments.

lib/Model/events.js Outdated Show resolved Hide resolved
Copy link
Contributor

@nateps nateps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor additional comments. LGTM with these resolved

lib/Model/events.js Outdated Show resolved Hide resolved
lib/Model/events.js Outdated Show resolved Hide resolved
lib/Model/events.js Outdated Show resolved Hide resolved
@ericyhwang ericyhwang merged commit aafd41d into master Jul 11, 2019
@ericyhwang ericyhwang deleted the vararg-alternatives branch July 11, 2019 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants