Make URLSearchParams compliant with the spec and browser #1055

ztplz · 2018-10-21T21:56:14Z

According to MDN Docs keys, values and entries shouldn't a generator function, and need to return a Iterator.
For example:
in chrome and firefox:

var us = new URLSearchParams()
undefined
console.log(us.keys.constructor.name)
Function
undefined
console.log(us.values.constructor.name)
Function
undefined
console.log(us.entries.constructor.name)
Function

kyranet · 2018-10-21T22:29:32Z

This needs tests to ensure this is spec compliant now

ztplz · 2018-10-21T22:55:22Z

@kyranet Sure.
Output in chrome:

var us = new URLSearchParams
undefined
var keys = us.keys()
undefined
Symbol.iterator in keys
true
Object.prototype.toString.call(keys)
"[object Iterator]"

ztplz · 2018-10-21T22:56:09Z

@kyranet Sure.
Output in chrome:

var us = new URLSearchParams
undefined
var keys = us.keys()
undefined
Symbol.iterator in keys
true
Object.prototype.toString.call(keys)
"[object Iterator]"

ztplz · 2018-10-21T22:57:31Z

@kyranet Sure.
Output in chrome:

var us = new URLSearchParams
undefined
var keys = us.keys()
undefined
Symbol.iterator in keys
true
Object.prototype.toString.call(keys)
"[object Iterator]"

ztplz · 2018-10-21T22:59:33Z

@kyranet Sure.
Output in chrome:

var us = new URLSearchParams
undefined
var keys = us.keys()
undefined
Symbol.iterator in keys
true
Object.prototype.toString.call(keys)
"[object Iterator]"

ry

LGTM

kitsonk · 2018-10-22T18:35:25Z

Generators are iterators. Generators are functions that codify iterators. Something like:

class Foo {
  *keys() {
    yield "foo";
    yield "bar";
  }
}

const foo = new Foo();
const keys = foo.keys();
console.log(Symbol.iterator in keys); // true

Is exactly the spec functionality, except that it logs out [object Generator] instead of [object Iterator] which I really argue is not something we should be concerned with, given that you get all the iterator functionality that is needed.

I think we need to align to what is being done in #1062 so until #1062 settles down and this gets realigned to that, we should hold off merging.

ztplz · 2018-10-22T19:10:34Z

@kitsonk Okay, I see. Thanks for your explanation.

ry · 2018-10-23T23:14:56Z

@ztplz @kitsonk Whats the status on this? It looks like it's not in the style Kitson is recommending?

kitsonk · 2018-10-24T00:05:18Z

@ry correct, it needs to follow the pattern established in #1062

hayd · 2018-10-24T00:10:45Z

and it needs tests

kitsonk

A few comments...

kitsonk · 2018-10-25T10:22:44Z

js/dom_types.ts

@@ -90,7 +90,7 @@ export interface ProgressEventInit extends EventInit {
  total?: number;
 }

-export interface URLSearchParams {
+export interface URLSearchParams extends DomIterable<string, string>{


I don't think this has been through the formatter again. Prettier would have inserted a space before the opening brace.

kitsonk · 2018-10-25T10:25:29Z

js/globals.ts

@@ -6,7 +6,7 @@ import { globalEval } from "./global_eval";
 import { libdeno } from "./libdeno";
 import * as textEncoding from "./text_encoding";
 import * as timers from "./timers";
-import * as urlSearchParams from "./url_search_params";
+import * as urlSearchParams_ from "./url_search_params";


you don't need to underscore this... it is only needed when there is a name collision in the global namespace, (e.g. console or fetch). Since there is no global variable named urlSearchParams it is safe to use.

My bad, I just thought it was the same as the browser.

kitsonk · 2018-10-25T10:27:16Z

js/mixins/dom_iterable.ts

@@ -24,19 +24,19 @@ export function DomIterableMixin<K, V, TBase extends Constructor>(
  // tslint:disable-next-line:variable-name
  const DomIterable = class extends Base {
    *entries(): IterableIterator<[K, V]> {
-      for (const entry of (this as any)[dataSymbol].entries()) {
+      for (const entry of (this as any)[dataSymbol]) {


This change is also in #1056. Maybe the best thing to do is create a seperate change that includes this that both #1056 and this one can base off of.

I don't think need to create a seperate change in here. for const entry of Array<k, v> as same as for const entry of Map<k, v>.

kitsonk · 2018-10-25T10:29:14Z

js/mixins/dom_iterable.ts

        yield entry;
      }
    }

    *keys(): IterableIterator<K> {
-      for (const key of (this as any)[dataSymbol].keys()) {
+      for (const [key,] of (this as any)[dataSymbol]) {


#1056 has this as:

Suggested change

for (const [key,] of (this as any)[dataSymbol]) {

for (const [key] of (this as any)[dataSymbol]) {

which is better

kitsonk · 2018-10-25T10:34:27Z

js/url_search_params.ts

+export const URLSearchParams = DomIterableMixin<string, string, typeof URLSearchParamsBase>(
+  URLSearchParamsBase,
+  params
+);


not run through the formatter or this would have been changed to break before EOF as well as on line 146 lacking spacing before the opening parenthesis.

kitsonk · 2018-10-25T10:35:21Z

js/url_search_params.ts

      .map(
        tuple =>
          `${encodeURIComponent(tuple[0])}=${encodeURIComponent(tuple[1])}`
      )
      .join("&");
  }
 }
+
+// @internal


There should be no need for @internal here.

kitsonk · 2018-10-25T10:36:27Z

js/url_search_params.ts

+
+// @internal
+// tslint:disable:max-line-length
+// tslint:disable-next-line:variable-name


Suggested change

// tslint:disable-next-line:variable-name

// tslint:disable-next-line:variable-name max-line-length

But even then, Prettier should break the generic arguments up.

So yeah, now with the wrapping after running through prettier, max-line-length is redundant.

ry

This was preemptively approved. Please address @kitsonk's comments.

kitsonk

One minor nit, but other than that LGTM.

Also, it should cleanly apply, but we have overlapping changes with #1056.

kitsonk · 2018-10-26T22:20:45Z

js/url_search_params.ts

+
+// @internal
+// tslint:disable:max-line-length
+// tslint:disable-next-line:variable-name


So yeah, now with the wrapping after running through prettier, max-line-length is redundant.

ztplz · 2018-10-29T02:04:56Z

@kitsonk Yes, the problem is think about how to reuse.

ry

LGTM - thank you - and sorry for the delay! Having some issues with appveyor build but will land once those are settled. I've rebased and squashed the branch.

ry · 2019-01-09T22:44:40Z

Closing old PRs. Please open a new one if this hasn't been resolved yet.

hayd · 2019-01-09T22:57:54Z

Looks like #1049 superseded this anyway?

kyranet · 2019-01-09T23:01:31Z

Looks like #1049 superseded this anyway?

No, #1049 is the original implementation, this PR fixed a bug where iterators aren't spec compliant.

ry approved these changes Oct 22, 2018

View reviewed changes

kitsonk reviewed Oct 25, 2018

View reviewed changes

ry requested changes Oct 26, 2018

View reviewed changes

kitsonk reviewed Oct 26, 2018

View reviewed changes

ry force-pushed the fix_url branch from 31fe991 to 1cf3065 Compare November 13, 2018 18:32

ry approved these changes Nov 13, 2018

View reviewed changes

ry mentioned this pull request Nov 16, 2018

Cargo and GN should build into same directory. #1184

Merged

Make URLSearchParams compliant with the spec and browser

fc6bd53

ry force-pushed the fix_url branch from 1cf3065 to fc6bd53 Compare November 17, 2018 07:14

ry closed this Jan 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make URLSearchParams compliant with the spec and browser #1055

Make URLSearchParams compliant with the spec and browser #1055

ztplz commented Oct 21, 2018

kyranet commented Oct 21, 2018

ztplz commented Oct 21, 2018

ztplz commented Oct 21, 2018

ztplz commented Oct 21, 2018

ztplz commented Oct 21, 2018

ry left a comment

kitsonk commented Oct 22, 2018

ztplz commented Oct 22, 2018

ry commented Oct 23, 2018

kitsonk commented Oct 24, 2018

hayd commented Oct 24, 2018

kitsonk left a comment

kitsonk Oct 25, 2018

kitsonk Oct 25, 2018

ztplz Oct 26, 2018

kitsonk Oct 25, 2018

ztplz Oct 26, 2018

kitsonk Oct 25, 2018

kitsonk Oct 25, 2018

kitsonk Oct 25, 2018

kitsonk Oct 25, 2018

ztplz Oct 26, 2018

kitsonk Oct 26, 2018

ry left a comment

kitsonk left a comment

kitsonk Oct 26, 2018

ztplz commented Oct 29, 2018

ry left a comment

ry commented Jan 9, 2019

hayd commented Jan 9, 2019

kyranet commented Jan 9, 2019

	for (const [key,] of (this as any)[dataSymbol]) {
	for (const [key] of (this as any)[dataSymbol]) {

	// tslint:disable-next-line:variable-name
	// tslint:disable-next-line:variable-name max-line-length

Make URLSearchParams compliant with the spec and browser #1055

Make URLSearchParams compliant with the spec and browser #1055

Conversation

ztplz commented Oct 21, 2018

kyranet commented Oct 21, 2018

ztplz commented Oct 21, 2018

ztplz commented Oct 21, 2018

ztplz commented Oct 21, 2018

ztplz commented Oct 21, 2018

ry left a comment

Choose a reason for hiding this comment

kitsonk commented Oct 22, 2018

ztplz commented Oct 22, 2018

ry commented Oct 23, 2018

kitsonk commented Oct 24, 2018

hayd commented Oct 24, 2018

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ztplz commented Oct 29, 2018

ry left a comment

Choose a reason for hiding this comment

ry commented Jan 9, 2019

hayd commented Jan 9, 2019

kyranet commented Jan 9, 2019