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

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

Open
evan-king opened this issue Aug 9, 2017 · 7 comments

Comments

@evan-king
Copy link
Collaborator

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);
}
@binki
Copy link

binki commented Aug 9, 2017

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.

@evan-king
Copy link
Collaborator Author

evan-king commented Aug 10, 2017

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. 😛

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.

@fent
Copy link

fent commented Aug 20, 2017

Makes more sense. Sounds good to me.

@evan-king evan-king modified the milestone: v12.0 Jan 4, 2018
@markddrake
Copy link

This is definitely driving me insane...

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

@markddrake
Copy link

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.

@corno
Copy link

corno commented Feb 4, 2020

What is the status of this issue?

@corno
Copy link

corno commented Feb 4, 2020

What is the status of this issue?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants