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

feat: Add --location=<href> and globalThis.location #7369

Merged
merged 3 commits into from
Jan 7, 2021

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Sep 6, 2020

op_crates/web/lib.rs Outdated Show resolved Hide resolved
@caspervonb
Copy link
Contributor

caspervonb commented Oct 9, 2020

Discussed this briefly before with @nayeemrmn in chat a while back but I'll repeat it here since this is still pending.

API Semantics are fine I think, but we might have to allow for this to change the origin of contexts, so if we can better to see this behind --unstable so that we aren't immediately locked into the flags semantics.

This came up recently again with an awkward hack for origins, I think we've barely pried the lid off this can of worms (origin related issues).

@nayeemrmn
Copy link
Collaborator Author

We need the flag either way so it can run web scripts out of the box. A similar thing came up with service workers: https://discord.com/channels/684898665143206084/684898665151594506/741039703943413873. This is the main priority for --location.

As for whether or not we want to allow changing the location at runtime down the line... I'd want to hear the use cases. I suspect they aren't isomorphic with the web.

@caspervonb
Copy link
Contributor

caspervonb commented Oct 9, 2020

We need the flag either way so it can run web scripts out of the box.

Yep thats fine, i'd just like to see the flag be unstable at first as all other new flags have been so we're not permanently locked into semantics we're just starting to unravel.

s for whether or not we want to allow changing the location at runtime down the line

Not at runtime, but an open question is if --location should be the default import origin when origin is None for example.

The referrer of import does change depending on the base of a web page AFAIK but isnt adressed here, so unstable to allow breaking revisions.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 11, 2020

Sorry I haven't paid as much attention to this as I should. A couple questions:

  • Can the URI be a local file (file:///path/to/file.html)
  • All of your examples in the documentation are just using just a hostname, with no path... I assume something like https://example.com/a/path/index.html is acceptable?

@kitsonk
Copy link
Contributor

kitsonk commented Oct 11, 2020

Another thought, especially for workers, it would be really handy to have a shortcut to set it to the cwd.

@lucacasonato
Copy link
Member

lucacasonato commented Oct 11, 2020

Can the URI be a local file (file:///path/to/file.html)

This is probably the biggest question yet. One main use cases for having a window location, is being able to determine the script origin. Getting an origin for a file URL is explicitly not specified: https://url.spec.whatwg.org/#origin. Currently we do the same as Chrome, which is give all file URLs the same origin: new URL("file:///a/b/c").origin == "file://". This sucks for web storage apis like localstorage, because all local scripts would share a storage bucket. Because of this I think we should disallow file:// URLs in --location, at least for the first pass.

This also rules out the cwd shortcut.

I assume something like https://example.com/a/path/index.html is acceptable?

Yeah, although https://example.com/a/path/index.html and https://example.com/b/foo/index.html would share the origin https://example.com, and would this share a storage bucket.

@nayeemrmn
Copy link
Collaborator Author

Can the URI be a local file (file:///path/to/file.html)

Yes, since you can get such a location by opening a file URL in a browser. It might be better to unsupport them at the level of origin-concerned consumers like localStorage. But I also don't mind removing this.

Another thought, especially for workers, it would be really handy to have a shortcut to set it to the cwd.

Like allowing relative paths? As explained in the last section of the documentation, I think this only makes bad usage easier. People already have a way of specifying relatively located worker URLs with import.meta.url. It's part of the reason I didn't want a default.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 11, 2020

But I also don't mind removing this.

Actually I think it is handy, and local storage should do whatever local storage does when the origin is a local file.

@lucacasonato
Copy link
Member

lucacasonato commented Oct 11, 2020

local storage should do whatever local storage does when the origin is a local file

The origin of a local file is always file://. That means all local files share a local storage bucket. That seems very, very unintuitive.

@kitsonk kitsonk requested a review from ry October 11, 2020 22:39
@kitsonk
Copy link
Contributor

kitsonk commented Oct 11, 2020

That means all local files share a local storage bucket. That seems very, very unintuitive.

Lots of web standards are unintuitive... 😄 But that is something to debate on a local storage PR. 😉

@lucacasonato
Copy link
Member

lucacasonato commented Oct 11, 2020

But that is something to debate on a local storage PR

I disagree - this is not something we can push to the future, as this problem is not local storage specific. The core problem here is determining the window origin from the window location. We have to be aware of the consequences of allowing file URLs as a window location when landing this PR. There is nothing we can do about it later in the local storage PR (or any other PR that uses window origin).

@kitsonk
Copy link
Contributor

kitsonk commented Oct 11, 2020

I disagree - this is not something we can push to the future, as this problem is not local storage specific. The core problem here is determining the window origin from the window location. We have to be aware of the consequences of allowing file URLs as a window location when landing this PR. There is nothing we can do about it later in the local storage PR (or any other PR that uses window origin).

But I am not sure what you are arguing or proposing. You can't magic up another origin out of thin air. While I agree that if you set location, that you share the same storage bucket for all code that shares the same origin might be surprising for some, it is logical and I don't see any other alternative. That is something to make clear in the documentation versus holding up what is consistent logic. The alternative is to disallow file:/// locations which seems illogical.

What we haven't addressed though would be how file://some-host/ would be handled versus file:/// URIs. That would give different origins, but are confusing for things like fetch, because "where" is that file?

@lucacasonato
Copy link
Member

all code that shares the same origin might be surprising for some

That isn't my point - that is indeed totally expected. The unexpected part is that all file URLs share an origin. Most people likely expect that file:///home/lucacasonato/myproject/index.html and file:///home/lucacasonato/secretproject/index.html don't share a local storage bucket.

Also file:// URLs would cause some other issues. If relative URLs in fetch based on the window location are possible, how would this interact with non http urls? We can't fetch file:// URLs at the moment.

My main issue is that file:// origins bring with it ton of edge cases. They are pretty much completely not standardised (see https://url.spec.whatwg.org/#origin) and their implementation and permissions vary greatly depending on user agent. I think we can figure this out in the long run, but for the short term it adds a lot of complexity for arguably very little value. I would prefer we do the initial implementation without support for file:// window locations, and then slowly tackle this after we figured out how this feature is used in the wild.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 11, 2020

Fair enough. My thought was the benefit to workers, but as @nayeemrmn points out, there are better ways. There is no harm in disallowing them for now, I agree.

@nayeemrmn nayeemrmn force-pushed the location branch 2 times, most recently from 9bab164 to 0643664 Compare October 20, 2020 13:58
@bartlomieju
Copy link
Member

Core team discussed --location flag on several occasions, but we were unable to reach a consensus.

Given that release is scheduled for Tuesday I think it's best solution to bump this PR to 1.6.0 (yes, I know it was already bumped from 1.4.0).

There is no immediate need for this flag in upcoming release and the first feature that requires it is the localStorage implementation which is not gonna make it for 1.5.0 anyway.

cli/flags.rs Show resolved Hide resolved
cli/flags.rs Outdated Show resolved Hide resolved
cli/standalone.rs Outdated Show resolved Hide resolved
cli/tests/unit/fetch_test.ts Show resolved Hide resolved
docs/runtime/workers.md Outdated Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - sorry for the very long delay @nayeemrmn and thank you for seeing this through.

@lucacasonato reports this is needed for fetch WPT tests, hence the sudden movement on this.

cli/flags.rs Outdated Show resolved Hide resolved
runtime/js/99_main.js Show resolved Hide resolved
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

One final comment.

runtime/js/11_workers.js Outdated Show resolved Hide resolved
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a bunch @nayeemrmn. Sorry this took so long!

@lucacasonato lucacasonato merged commit e61e81e into denoland:master Jan 7, 2021
@nayeemrmn nayeemrmn deleted the location branch January 7, 2021 19:13
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.

discussion: --location=<URL> option
6 participants