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

Replace Location class with browser-provided URL #90

Merged
merged 4 commits into from
Jan 21, 2021

Conversation

seanpdoyle
Copy link
Contributor

The Turbo-internal Location class' responsibilities overlap almost
entirely with the browser-provided URL class. In addition, Turbo's
Location class name conflicts with the browser-provided Location
class, which is itself an almost URL-compliant interface.

This commit removes the Turbo-internal Location class, and modifies
all consuming interfaces to use URL instead.

The properties and functions that do not have corresponding
URL-provided versions have been extracted to the src/core/url.ts
module.

src/core/url.ts Outdated Show resolved Hide resolved
@Intrepidd
Copy link
Contributor

👍 I was about to open a ticket to ask to export it as it is needed by navigator.history.push which is exported, this is even better, thanks !

The Turbo-internal `Location` class' responsibilities overlap almost
entirely with the browser-provided [URL][] class. In addition, Turbo's
`Location` class name conflicts with the browser-provided [Location][]
class, which is itself an almost `URL`-compliant interface.

This commit removes the Turbo-internal `Location` class, and modifies
all consuming interfaces to use `URL` instead.

The properties and functions that do not have corresponding
[URL][]-provided versions have been extracted to the `src/core/url.ts`
module.

[URL]: https://developer.mozilla.org/en-US/docs/Web/API/URL
[Location]: https://developer.mozilla.org/en-US/docs/Web/API/Location
Incorporate the anchor change in behavior from [hotwired#58][].

[hotwired#58]: hotwired#58
Copy link
Contributor

@sstephenson sstephenson left a comment

Choose a reason for hiding this comment

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

Cheers to modernization! 🎉

src/core/drive/visit.ts Outdated Show resolved Hide resolved
src/core/url.ts Outdated Show resolved Hide resolved
src/core/url.ts Outdated Show resolved Hide resolved
src/core/url.ts Outdated Show resolved Hide resolved
src/core/url.ts Outdated Show resolved Hide resolved
src/http/fetch_request.ts Outdated Show resolved Hide resolved
src/http/fetch_request.ts Outdated Show resolved Hide resolved
src/http/fetch_request.ts Outdated Show resolved Hide resolved
src/http/fetch_request.ts Show resolved Hide resolved
src/core/frames/frame_controller.ts Outdated Show resolved Hide resolved
@sstephenson sstephenson merged commit f9d1651 into hotwired:main Jan 21, 2021
@seanpdoyle seanpdoyle deleted the use-browser-url branch January 21, 2021 19:21
@ghiculescu
Copy link

Hey, I think there is a bug in how forms are submitted since this PR was merged. Basically, all the elements of a form are being included in the URL when it's submitted.

This is what form submissions look like with the latest Turbo from npm:
image

If you clone the repo and then point your package.json at your clone on the main branch, they look like this:

image

If you do git checkout 178c817dc75d9559b3b624058f28a5bbf466a841 then run against that, the bug is gone:

image

This line seems to be the culprit: https://github.com/hotwired/turbo/pull/90/files#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R53

It looks like it is replacing the old behavior here, but that behavior only ran on GET requests. Now it runs on every request.

@seanpdoyle
Copy link
Contributor Author

@ghiculescu thanks for reporting this! I believe the decision was deliberate, but #128 might handle the nuance a little more appropriately.

@ghiculescu
Copy link

I don't think putting the contents of the form body in the URL are a good idea. As an example of why, different browsers implement different rules around URL lengths which this could now break. There's probably also security implications, eg. if your server logs URLs but not request bodies, it may suddenly start logging passwords or authenticity tokens (like my 2nd screenshot).

@ghiculescu
Copy link

ba323e4 looks like it'll fix it. Thanks @seanpdoyle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants