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(ext/webstorage): use implied origin when --location not set #12548

Merged
merged 3 commits into from
Oct 27, 2021

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Oct 26, 2021

Closes #11882

This differs a little bit from the proposal, in that there is no window.location set at all when --location is not supplied. That behaviour stays the same. Instead though, the storage origin is determined fully on the Rust side and not exposed at all in the runtime. The algorithm is:

  • Use the origin of the --location if a --location is passed (unless the --location has an opaque origin, and in this case localStorage will be disabled)
  • If no --location is passed, but there is a config file, use the absolute path to the config file as the "origin", meaning that programs that share the same config file, will share the same localStorage.
  • If no configuration file is present, use the URL of the main module. Currently for the REPL and test, a synthetic module is generated based on the cwd when Deno starts. This means executing the REPL from the same cwd means subsequent invocations will share storage.

I need to write tests for it. Also noticed that inspecting window.localStorage or window.sessionStorage isn't working, because the proxy doesn't properly deal with the inspect symbol.

I will also add documentation to the manual once we have merged this PR.

This is going to be a little bit of a breaking change, because we were setting the storage path based on the full --location and not the origin of the --location, meaning that --location https://deno.land/x/a.ts and --location https://deno.land/x/b.ts would not share the same storage, but to be well aligned to the web platform, they should. Between the config path and the main module implied storage origins, there should be sufficient alternatives to allow flexibility so that we can better align to the web platform.

@lucacasonato
Copy link
Member

lucacasonato commented Oct 26, 2021

There is some conflicting info in the PR description:

Use the origin of the --location if a --location is passed

This is going to be a little bit of a breaking change, because we were setting the storage path based on the full --location and not the origin of the --location

Which one is it? Should be the first IMO. EDIT: code says it is the first one, so doesn't look like a breaking change to me.

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. We definitely need to add docs for this to the manual.

cli/main.rs Outdated Show resolved Hide resolved
@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 26, 2021

@lucacasonato currently (main) the full --location URL as a string is being used, the PR changes it to use the ascii serialized version of the origin --location URL.

@lucacasonato
Copy link
Member

Oh I see. Good catch.

cli/main.rs Show resolved Hide resolved
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.

Internal Origin for Persistence
3 participants