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

URLSearchParams: calling .set() and .append() should return itself #90

Closed
marcoscaceres opened this issue Feb 15, 2016 · 14 comments
Closed
Labels
needs implementer interest Moving the issue forward requires implementers to express interest topic: api

Comments

@marcoscaceres
Copy link
Member

Given a list of search params (strings) to be inserted into a URLSearchParams object, it would be useful if calling .set() and .append() would return the instance of URLSearchParams. Two reasons:

  1. URLSearchParams, although not "map-like", is extremely close to an ES Map in look and feel. ES Map() returns itself when a developer calls set(). Similarly, so do the calls on ES Set(). It is surprising that URLSearchParams does not work the same.
  2. Using listOfURL.reduce() (or other functions that operate on lists that expect an object in return) would benefit from this.

Consider, this would allow the following code:

function buildQuery(items) {
  const params = new URLSearchParams();
  items
    .map(item => item.url)
    // This is not great, requires unnecessary "{ }", ";" x2, and "return"  :(
    .reduce((params, url) => {
       params.append("foo", url);
       return params;
    }, params)
  return params.toString();
}

Could become much cleaner:

function buildQuery(listOfThings) {
  return items
    .map(listOfThings => listOfThings.url)
    .reduce((params, url) => params.append("foo", url), new URLSearchParams())
    .toString();
}

//cc @domenic

@marcoscaceres
Copy link
Member Author

(fixed up first code example above)

@annevk
Copy link
Member

annevk commented Feb 15, 2016

This pattern of returning this is not something TC39 is still advocating I believe. That's why we didn't copy it. It makes it impossible to introduce a useful return value in the future.

@marcoscaceres
Copy link
Member Author

Ok, sent to public-script-coord, to hopefully get a little more ocntext. Closing for now.
https://lists.w3.org/Archives/Public/public-script-coord/2016JanMar/0096.html

@marcoscaceres
Copy link
Member Author

Reopening, as there was support on public-script-coord for adding this.

@marcoscaceres marcoscaceres reopened this Mar 12, 2016
@annevk
Copy link
Member

annevk commented Dec 19, 2016

The reason I haven't done this yet by the way is that the support was not overwhelming and there's other differences between Set/Map and URLSearchParams/FormData/Headers (such as the latter returning null). It'll also require some implementation and testing effort.

@marcoscaceres
Copy link
Member Author

It would be good to do it for all of them tho (and because they currently return undefined, it might mean that this can be done in a somewhat backwards compatible way).

This not returning self might become an issue - in as far that it's another place where the platform doesn't match ES! - as more people start migrating to ES6 and beyond, because all other map/set interfaces return self. This makes these interfaces annoying to use when writing functional code.

Would it help to get more opinions from functional programming folks?

@annevk
Copy link
Member

annevk commented Dec 20, 2016

What returns undefined currently?

I actually think not matching ES is a feature here. If we ever wanted to return something meaningful, we still have the option to do so...

@marcoscaceres
Copy link
Member Author

What returns undefined currently?

.append() and friends.

I actually think not matching ES is a feature here.

I'd be interested to hear how it's a feature to break with ES? What are the benefits for devs?

@annevk
Copy link
Member

annevk commented Dec 20, 2016

Oh, I was talking about get() returning null rather than undefined (as it does in ES).

I'd be interested to hear how it's a feature to break with ES?

As was pointed out on public-script-coord what ES does doesn't make much sense. It's totally unclear what methods will return this and what methods will return something else. And if we ever got to a point where append() might not succeed, we cannot indicate success through a return value anymore.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Dec 20, 2016

Oh, I was talking about get() returning null rather than undefined (as it does in ES).

Ah. Yeah, that can be nice when the multi-map is typed.

As was pointed out on public-script-coord what ES does doesn't make much sense. It's totally unclear what methods will return this and what methods will return something else.

I agree that for some APIs that might be the case, but there is only a limited set of operations for Map/Set-like things. It's pretty easy to map them to ES and match the behavior (e.g., when you add and append stuff).

And if we ever got to a point where append() might not succeed, we cannot indicate success through a return value anymore.

Sure, but that would be fairly exceptional... thus throw, no?

Anyway, if it's just me that things it's a bit annoying then I'm ok to close this - but I keep hitting it. Wish we had some way of gathering better developer feedback about this kind of thing. Running a totally unscientific poll about it:
https://twitter.com/marcosc/status/811114887959048196

@annevk
Copy link
Member

annevk commented Dec 20, 2016

Another thing I was thinking, for a multimap you might want append() and such to return something indicating whether the multimap already contains the key being appended. It's not clear to me what utility returning this offers and the Twitter poll doesn't really address that point either.

@annevk annevk added topic: api needs implementer interest Moving the issue forward requires implementers to express interest labels Dec 20, 2016
@marcoscaceres
Copy link
Member Author

Another thing I was thinking, for a multimap you might want append() and such to return something indicating whether the multimap already contains the key being appended.

That seems to duplicate what .has() and .contains() is for - or possibly even .getAll(item).length.

It's not clear to me what utility returning this offers and the Twitter poll doesn't really address that point either.

It's what @jussi-kalliokoski said on Twitter: "consistency and least surprise". The current design violates both, because:

  1. Consistency: it's inconsistent with the language primitives (map and set).
  2. Least surprise: it's surprising that it doesn't return self, particularly when performing a reduce() as per the code below (which goes to argument of utility).

Consider again what we have today:

    .reduce((params, url) => {
       params.append("foo", url);
       return params;
    }, params)

So, the utility in returning self is that .reduce() expects the "collector" to be returned. Thus, returning this would be idiomatic because it would work like regular maps and sets:

  // Look ma! no return keyword and no "{" or "}"  
  .reduce((params, url) => params.append("foo", url), new URLSearchParams())

It's extremely common to want to .map() a list of things, and then .reduce() those things into a collection type, like URLSearchParams, DOMTokenList, Headers, etc. but it's surprising and inconsistent that doing so don't follow ES.

@marcoscaceres
Copy link
Member Author

the Twitter poll doesn't really address that point either.

To be clear, the point of the poll is to find out of developers actually give a crap (or not!) about these things - I know I do, which is why I raised this issue. If other web devs don't really care about consistent API surface across Web APIs, then this is a non-issue.

I know it's out of scope of this bug, but is there a better way for us to find this stuff out from Web developers? I feel like when I raised things as a web developer there is an implicit "but Marcos, you are not a REAL web developer: you are just some clown on the Internet who sends a lot of emails, so you don't really count."

Is there a trusted set of Real Web Developers™️ whose opinions would be of value here?

@annevk
Copy link
Member

annevk commented May 4, 2020

I'm going to close this as this hasn't really come up again in the intervening years and everyone seems wary of making changes to this API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs implementer interest Moving the issue forward requires implementers to express interest topic: api
Development

No branches or pull requests

2 participants