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.

Sign up for GitHub

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

Jump to bottom

window.location should be removed #3520

Closed
nayeemrmn opened this issue Dec 18, 2019 · 12 comments · Fixed by #5034
Closed

window.location should be removed #3520

nayeemrmn opened this issue Dec 18, 2019 · 12 comments · Fixed by #5034
Assignees
@bartlomieju
Labels
high priority

Comments

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Dec 18, 2019 •
edited
Loading

Currently window.location is the URL of the main module. A while ago @kitsonk agreed with me that this isn't ideal.

It should be the CWD with a trailing slash as a file URL. The CWD is where resources get resolved from so it is actually compatible with the current document in browsers. It would lend some sanity to things like #2150 and be part of a clearly isomorphic set of APIs.

It should be removed outright. See #3520 (comment) and #3520 (comment).
@sholladay
Copy link

sholladay commented Dec 31, 2019 •
edited
Loading

In that case, should Deno.cwd() be removed? Generally, Deno's philosophy has been to avoid having proprietary APIs unless there is a good reason for them to exist.

I guess you could argue that Deno.cwd() is slightly different in that it returns a path without a trailing slash. Is that enough to justify its existence, though? I don't think the trailing slash would generally cause any problems. All of the APIs I can think of handle it fine.

Also, how do we get the URL of the main module, after this change?

@sholladay sholladay mentioned this issue Jan 12, 2020
Closed
@bartlomieju
Copy link
Member

bartlomieju commented Feb 5, 2020 •
edited
Loading

@ry this should be figured out before 1.0

CC @kitsonk

@bartlomieju bartlomieju self-assigned this Feb 5, 2020
@ry ry mentioned this issue Feb 5, 2020
Closed
43 tasks
@kt3k
Copy link
Member

kt3k commented Feb 8, 2020

@nayeemrmn Can you give us some examples to show how this proposal works better than current implementation?

@nayeemrmn
Copy link
Collaborator Author

This is about the compatibility problem with the current usage of globalThis.location. The URL of the Deno main module isn't analogous with the URL of the web page. The Deno CWD is analogous to the URL of the web page in that they're both where resources are resolved from (e.g. resources as referenced using the fetch API).

The current one offers no interoperability, and yet it's disguised as a web API in the global namespace. The proposal offers interoperability for any code that assumes globalThis.location as the resource resolution root. If that's too small of an intersection then we should remove globalThis.location. It can't stay the way it is.

@nayeemrmn nayeemrmn changed the title window.location should be the CWD window.location should be the CWD (or removed) Feb 8, 2020
@bartlomieju
Copy link
Member

It turns out it is even more complicated...

// index.html
<html>
<head>
...
</head>
<body>
    <script src="./index.js"></script>
</body>
</html>

// index.js
console.log("main location", window.location.toString());
new Worker("./test_worker.js");

// test_worker.js
console.log("worker location", globalThis.location.toString());

Chrome, loaded from http URL:

main location http://localhost:8080/index.html
worker location http://localhost:8080/test_worker.js

Chrome, loaded from file URL:

Uncaught DOMException: Failed to construct 'Worker': Script at 'file:///[REDACTED]/test_worker.js' cannot be accessed from origin 'null'.

@nayeemrmn
Copy link
Collaborator Author

Hmm, we could certainly easily match the worker behaviour in either case but not handling file URLs? That's totally not going to work. Removal it is.

@nayeemrmn nayeemrmn changed the title window.location should be the CWD (or removed) window.location should be removed Feb 21, 2020
@bartlomieju
Copy link
Member

Hmm, we could certainly easily match the worker behaviour in either case but not handling file URLs? That's totally not going to work. Removal it is.

l actually believe that our current value is fine; URL Web API resolution yields proper results...

@nayeemrmn
Copy link
Collaborator Author

Yes the file URL case only breaks my initial proposal, not the current behaviour.

URL Web API resolution yields proper results...

As I've explained, there is no usable relationship between Deno's main module URL and the web's page URL. The assumed meaning of this API on the web is something Deno has no concept of. Can anyone give an isomorphic use case?

@bartlomieju
Copy link
Member

Update: we've discussed this issue with @piscisaureus and @ry and came to conclusion that even though Deno's window.location is not 100% aligned to what you get in Web browser it's actually ok to leave as is. In web browser window.location gives you location of document for the web page; in Deno there is no document but only an entry script. The behavior is the same for web workers.

That said; we'll need to update module resolution algorithm for local files before this issue can be closed (ref #3401)

@bartlomieju bartlomieju reopened this Mar 25, 2020
@bartlomieju bartlomieju mentioned this issue May 1, 2020
Merged
@kt3k
Copy link
Member

kt3k commented May 2, 2020

@bartlomieju Can you provide the reasoning behind the decision (removal of location)? Is this about the security concern?

@bartlomieju
Copy link
Member

@kt3k long story short; in browser window.location point to the address in the navbar; in Deno it always pointed to main script. Then self.location (the one in workers) behaved just like in browser. We decide to remove window.location for now, as our implementation and value is non-spec compliant and we don't want to be forced to support it forever. (We might still bring it back in some time, but it shouldn't be in 1.0)

@kt3k
Copy link
Member

kt3k commented May 2, 2020

@bartlomieju Thank you very much! Makes sense to me.

@nayeemrmn nayeemrmn mentioned this issue Sep 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bartlomieju bartlomieju

Labels
high priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

BREAKING: remove window.location and self.location bartlomieju/deno
4 participants
@kt3k @sholladay @bartlomieju @nayeemrmn