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

discussion: --location=<URL> option #4981

Closed
nayeemrmn opened this issue Apr 29, 2020 · 6 comments · Fixed by #7369
Closed

discussion: --location=<URL> option #4981

nayeemrmn opened this issue Apr 29, 2020 · 6 comments · Fixed by #7369
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted) suggestion suggestions for new features (yet to be agreed) web related to Web APIs

Comments

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Apr 29, 2020

In #1657 (comment) an --origin=https://example.com option is proposed to select which localStorage isolate is used by the process. This could also be used for lots of other purposes.

For a consistent analogy we should extend this to a --location=https://example.com/<path> which selects the location for all web API purposes, including but not limited to:

  • window.location.
  • fetch() relative resolution root.
  • Worker module relative resolution root.
  • localStorage origin.
  • Origin header for any built-in HTTP requests.
  • Base URL for inline import maps.

Without an explicit --location, globalThis.location should throw, localStorage should be non-persistent and document-relative URLs for the other purposes should fail. This makes --location act as the gate requested for persistent storage: #1657 (comment).

Not having a default might be controversial, but this lets us abstain from any opinionated design decisions for the first version.


JS API:

Object.defineProperty(globalThis, "location", {
  get() {
    if (locationFromFlag == null) {
      throw new ReferenceError(
        `Access to "globalThis.location", run again with --location=<href>.`,
      );
    }
    return locationFromFlag;
  },
  // A similar setter would be applied to fields of `locationFromFlag`.
  set() {
    throw new TypeError(`Cannot set "globalThis.location".`);
  },
});
@bartlomieju bartlomieju added cli related to cli/ dir feat new feature (which has been agreed to/accepted) suggestion suggestions for new features (yet to be agreed) web related to Web APIs labels May 21, 2020
@kitsonk
Copy link
Contributor

kitsonk commented Sep 1, 2020

I hadn't paid attention to this. I think it is an inevitability to be able to provide full API capabilities for the web.

I know we had debates before around this for window.location but what is the challenge/issue with this defaulting to file:///some/path/main.ts, being whatever the main module is for the execution?

@nayeemrmn
Copy link
Collaborator Author

I know we had debates before around this for window.location but what is the challenge/issue with this defaulting to file:///some/path/main.ts, being whatever the main module is for the execution?

The reason for #3520 was that we were squatting a web API for something that has no isomorphic usage just because we could. However, at the time we were talking about "using globalThis.location to expose the main module URL" 😠 Now we're talking about "using the main module URL as a convenient default for globalThis.location" 🤔

Nevertheless, it's scary and opinionated to assign our own default to a web API and I seriously question that we need one.

When we unsupported this for Workers, people seeking help with the breakage didn't really know what their relative paths were being based onto in the first place. The default resolution aligned with their unmindful expectation, so they wrote code that was dependent on an environmental factor without realising. We should reduce these. By now it's been proven that this "convenience" isn't needed in favour of what people do now: new URL(path, import.meta.url).

Let's instead invite people to use --location when they know what they're doing with it. The use cases I had in mind were:

  • Emulating your site while testing a frontend component.
  • Specifying which localStorage isolate to use with fake origins as IDs.
  • Shutting up a portable script which is frivolously referencing globalThis.location.
  • Being able to use "convenient" relative paths for the listed web APIs 😱 But you've explicitly opted in with a flag and are aware that this will only work at application level.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 1, 2020

That is logical. I agree people mis-understanding what they thought they needed when using a web API, and I can see the logic that there is no good universal rule of what it should be, or what someone needs it to be, and actually a location that is a reference to a local file would be unusable for most folks and not really help portability of code. So a default being "wrong" most of the time doesn't make for a good default.

I think we would want some good rules about behaviour with it missing though, and would be especially useful if a hint was given "why don't you try that again with --location?"

@nayeemrmn can you think of any risks with allowing this to be set from within the sandbox? Like allowing window.location to be set in a browser, but potentially cancelled?

@nayeemrmn
Copy link
Collaborator Author

I landed on this, for maximum forward-compatibility:

Object.defineProperty(globalThis, "location", {
  get() {
    if (locationFromOpStart == null) {
      throw new ReferenceError(
        `Access to "globalThis.location", run again with --location=<href>.`,
      );
    }
    return locationFromOpStart;
  },
  // A similar setter would be applied to fields of `locationFromOpStart`.
  set() {
    throw new TypeError(`Cannot set "globalThis.location".`);
  },
});

This would make scripts that feature-test for window.location throw, which hurts. I think making it nullable would lead to type conflicts so we can't do that. I would just count this under the "Shutting up a portable script which is frivolously referencing globalThis.location" use case.

When setting, I was going to say that there's an expectation of navigation so we should throw rather than silently doing nothing. But I guess there are ways of setting it without unloading (e.g. window.history.pushState()). Nevertheless, we can't go wrong with throwing for now.

@lucacasonato
Copy link
Member

Here are some use-cases which I have that would be directly solved by this issue:

Isomorphic Server Side Rendering

Deno is already a great choice for isomorphic server side rendering because it implements many web APIs like fetch. Currently some of these APIs have limitations which require the user to do some trickery to get them fully isomorphic. One example of this is relative fetch. Currently the call fetch("/api/me") would fail on Deno, but would succeed client side on my page. To make this code isomorphic right now, I have the following workaround:

// server side generation bootstrap script
window.location = new URL("https://myfakepage.com/foo/bar");

// actual server side component
const res = await fetch(new URL("/api/me", window.location));

With --location, window.location would already be set for me, but that doesn't really matter because the relative fetch would just work as expected. Also being able to just use window.location during server side generation without having to do poly-filling would be awesome.

@piscisaureus
Copy link
Member

Reading material: whatwg/html#1888

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted) suggestion suggestions for new features (yet to be agreed) web related to Web APIs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants