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

default noEmptyComposite to true #499

Open
raylu opened this issue Jul 23, 2022 · 4 comments
Open

default noEmptyComposite to true #499

raylu opened this issue Jul 23, 2022 · 4 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@raylu
Copy link

raylu commented Jul 23, 2022

as #102 shows, the fact that JsonURL doesn't round-trip empty arrays is pretty surprising. I can't really think of a situation where you would want to save the : byte versus being able to round-trip the correct type

(I also wouldn't mind if AQF were the default since that's the main usage of this library, I think, but I don't know if that might have some backwards compatability problems)

@dmaccormack
Copy link
Collaborator

Hi, @raylu. I understand your point of view, but it comes down to backwards compatibility. When I cut a 2.x revision I can revisit defaults. I agree that the inability to roundtrip is counterintuitive, so noEmptyComposite will probably default to true. I'm less convinced about changing AQF, though. That can have a big affect on the size of encoded text.

Unfortunately, I don't really have data about how users change defaults to base these sorts of decisions on.

@dmaccormack dmaccormack self-assigned this Jul 24, 2022
@dmaccormack dmaccormack added the enhancement New feature or request label Jul 24, 2022
@dmaccormack dmaccormack added this to the v2.0.0 milestone Jul 24, 2022
@raylu
Copy link
Author

raylu commented Jul 24, 2022

are you worried about people depending on the behavior of serializing existing empty arrays as ()? noEmptyComposite doesn't affect deserialization of existing strings, right?

at the least, I think this should be mentioned on the README. I saw the thing about AQF so I enabled it, but I really would have liked to know about noEmptyComposite without having to dig through the source

(I'm curious why you're against defaulting AQF to on. isn't the entire point of JsonURL to work for URLs? are people passing them around without them hitting the address bar at some point? or did I misunderstand what AQF is for?)

@Xample
Copy link

Xample commented Aug 30, 2022

Hello, I just started making a POC with this lib. As I'm sometimes passing empty arrays in filters, I agree that the bijection of JSON -> JSONUrl -> JSON is more than mandatory to get rid of surprises. I will check the noEmptyComposite flag

@dmaccormack
Copy link
Collaborator

dmaccormack commented Oct 19, 2022

are you worried about people depending on the behavior of serializing existing empty arrays as ()? noEmptyComposite doesn't affect deserialization of existing strings, right?

noEmptyComposite changes the meaning of () when it is parsed. When noEmptyComposite is false () is parsed as the empty composite and assigned a potentially caller provided value. When noEmptyComposite is true () is parsed as an empty array. It does affect the deserialization of existing strings.

at the least, I think this should be mentioned on the README. I saw the thing about AQF so I enabled it, but I really would have liked to know about noEmptyComposite without having to dig through the source

A fair point and good suggestion. Thanks!

(I'm curious why you're against defaulting AQF to on.

Enabling AQF by default in version 2.0 is a breaking change would affect every single user. And any backend code that's in place would need to be changed at the same time. It could be very problematic in practice.

It will also increase the size of stringified JSON→URL text. One user in the past has noted that size was a contributing factor when choosing JSON→URL over alternatives (#97 (comment)).

isn't the entire point of JsonURL to work for URLs? are people passing them around without them hitting the address bar at some point? or did I misunderstand what AQF is for?)

No, you did not misunderstand. That is what AQF is for, but not everyone uses JSON→URL in a browser address bar. It works well for idempotent server-side HTTP API calls too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants