Skip to content

Add URL.searchParams (URLSearch​Params) #361

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

Merged
merged 2 commits into from
May 14, 2019
Merged

Conversation

exoego
Copy link
Contributor

@exoego exoego commented May 13, 2019

Closes #339

URLSearch​Params is widely-adopted standard.

* MDN
*/
@js.native
trait URLSearchParamsInterface extends js.Object {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't seem to exist an URLSearchParamsInterface even in the IDL definitions. Consider just defining the class URLSearchParams.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1db81ff

It was separated as MDN says URLSearchParams has separate "interface" and constructor.
But, same name between trait and class is not allowed in Scala, so it was suffixed with ~Interface.
By separating trait, we can have our own class that implements URLSearchParams interface.

However, I change my mind to just define class, since URL is defined so.

}

/**
* The URLSearchParams() constructor creates and returns a new URLSearchParams object. Leading '?' characters are ignored.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wrap lines at 80 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this line during merging trait and class.

*/
@js.native
@JSGlobal
class URLSearchParams extends URLSearchParamsInterface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class should extend js.Iterable[js.Tuple2[String, String]].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

def entries(): js.Iterator[js.Tuple2[String, String]] = js.native

@JSName(js.Symbol.iterator)
def values(): js.Iterator[String] = js.native
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those two methods with different signatures and the same @JSName make no sense. I think what you wanted is a single def iterator(): js.Iterator[js.Tuple2[String, String]] to conform to the interface of js.Iterable[js.Tuple2[String, String]]. The methods entries() and values() should not have any @JSName. It also seems that you're missing the def keys() method.

See https://heycam.github.io/webidl/#idl-iterable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

Implemented js.Iterable[js.Tuple2[String, String]] and used @JSName to override def jsIterator.
keys also added.

@exoego exoego force-pushed the searchParams branch 2 times, most recently from 1db81ff to e650ea9 Compare May 13, 2019 21:50
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unrelated change made it into the PR, which should not be there. Otherwise good to go.

@@ -276,7 +276,7 @@ object Ajax {
} else {
// fall back to copying the data
val tempBuffer = ByteBuffer.allocateDirect(data.remaining)
val origPosition = data.position
val origPosition = data.position()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is unrelated and should not be in this commit/PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, indeed.
Since it was fixed in another PR, I will rebase and exclude unrelated change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed !!

@sjrd sjrd merged commit e9ae7ad into scala-js:master May 14, 2019
@exoego exoego deleted the searchParams branch May 14, 2019 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add URLSearchParams api
2 participants