-
-
Notifications
You must be signed in to change notification settings - Fork 835
Correct changelog entry for arrayLimit fix #538
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
base: main
Are you sure you want to change the base?
Conversation
- should warn that arrayLimit is a breaking change
| @@ -1,5 +1,5 @@ | |||
| ## **6.14.1** | |||
| - [Fix] ensure arrayLength applies to `[]` notation as well | |||
| - [Fix] (breaking change) ensure arrayLimit applies to `[]` notation as well | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is absolutely not a breaking change; bugfixes can sometimes break people if they're depending on broken behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo it doesn't matter if behaviour was broken or not. What matters is that this is a significant behaviour change.
Idk but feels like you're being defensive or smth. It's not like i am blaming you for broken apps (it wouldn't make any sense) or asking to release new major version because of it. It's me trying to warn other devs, so people updating to 6.14.1 will be aware that it can break their apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I agreed it was a breaking change, then I would be compelled to revert it, and publish a new major, and tons of people on v6 would be left vulnerable.
I don't agree it's a breaking change, which is why it's fine to release it in a non-major. EVERY change could break someone's app; that's what tests and lockfiles are for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got you. What wording would allow to warn people but not force you to publish new major? Would you be ok with something like:
- [Fix] ensure arrayLimit applies to `[]` notation as well.
WARN: if you handle more than `arrayLimit` (20 by default) items in array query param (i.e. `param[]=a&...param[]=x`) it will be converted to object instead of array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any warning is required, personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change introduced a breaking behavior for our application. We use repeated query params for multi-select filters: ?org=uuid1&org=uuid2&org=uuid3...
This broke our GraphQL queries which expect arrays.
While the fix is technically correct, this is a breaking change for apps relying on the previous behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, any change whatsoever could cause an arbitrary application to break. That's not the definition of "breaking change", and if it were, then literally everything would be semver-major every time.
The issue was that your code expected arrays all the time. That's never been a guarantee and arrayLimit should have applied, but incorrectly wasn't. The "failure" is actually a correct exposure of a bug in your own code - you're welcome!
arrayLimitbecause it's what devs use andarrayLengthis internal variable