clarinet icon indicating copy to clipboard operation
clarinet copied to clipboard

First key in object handled differently (in `openobject` event rather than `key` event)

Open evan-king opened this issue 8 years ago • 7 comments

As previously reported with a documentation clarification (see #46).

It seems to me that API usability would be improved by eliminating this special case, such that openobject only signifies the start of an object and key events be raised exactly once for every key without exceptions, starting in a v1.0 release (simultaneously committing to full and meaningful semver compliance). However I would appreciate feedback from @dscape as well as current maintainers of other dependent public packages.

Alternatively it might be nice to offer some more detailed usage guidance including coverage of edge cases. Here's an excerpt from a module I wrote—using clarinet and convertings its output to a top-level object/array stream—where I internally capture and normalize the current behavior:

function setKey(key) {
    // update current key
}
sax.onkey = setKey;

sax.onopenobject = function(key) {
    // track object creation

    if(key !== undefined) setKey(key);
}

evan-king avatar Aug 09 '17 20:08 evan-king

It is definitely something which I’d love to see changed in a major version. But the change as you described it might be too dangerous for even a major bump.

One safer, though more complicated and ugly, approach is to keep the existing events for backwards compatibility and add an oneverykey event which fires both for onkey and openobject. Or, if dropping the old events in the new major version (to keep things simpler), use different names for the events with the new behavior so that package maintainers notice something changed when upgrading to the latest major. Could even use Object.defineProperty() to mark onkey and onopenobject as read-only so that maintainers migrating to the new version can more quickly identify and update old code. Otherwise they may accidentally introduce subtle bugs that go unnoticed when updating to the next major.

binki avatar Aug 09 '17 22:08 binki

I'm inclined to blame the package consumer if they've configured their dependencies to automatically update to newer major versions. I'm also not overly keen on adding significant complexity to support configurability, which then must rely on compiler optimizations to (hopefully) recover branch/jump performance costs added.

On the other hand, if no other changes are made in a major version bump, then the version number is the configuration parameter. :stuck_out_tongue:

I like the idea of new event names, but wouldn't want to introduce a legacy of awkward event naming because some time in the past the "right" or intuitive names were linked to deprecated behavior.

evan-king avatar Aug 10 '17 15:08 evan-king

Makes more sense. Sounds good to me.

fent avatar Aug 20 '17 01:08 fent

This is definitely driving me insane...

I would expect OpenObject -> KeyName -> OpenOjbect /OpenArray/Value

markddrake avatar Oct 09 '18 19:10 markddrake

And maybe an option to control the behavior with the default to be the current behavior for some defined period (to give people a chance to change code), and the at some point switch to the new behavior by default with the option being used to switch to the old behavior.

markddrake avatar Oct 09 '18 19:10 markddrake

What is the status of this issue?

corno avatar Feb 04 '20 01:02 corno

What is the status of this issue?

I decided to fork clarinet into 'bass-clarinet' to solve this issue.

corno avatar Feb 04 '20 21:02 corno