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

fix(#10825): location should be undefined without --location #10828

Closed
wants to merge 5 commits into from

Conversation

ry
Copy link
Member

@ry ry commented Jun 3, 2021

fixes #10825

@ry ry changed the title fix(#10825): location and localStorage should be undefined without --… fix(#10825): location should be undefined without --location Jun 3, 2021
Comment on lines +365 to +367
console.warn(
"Warning: accessing undefined 'location' global, " +
"run again with --location",
Copy link
Member

Choose a reason for hiding this comment

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

This seems unfortunate... typeof location !== "undefined" would result in a warning logged to the console.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can remove it - I wanted to address @crowlKats's concern.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Luca... OTOH it will be hard to discover location API unless you plough through manual. I guess in this situation not showing warning is preferable

Copy link
Member Author

Choose a reason for hiding this comment

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

I would suggest that we leave the warning for the time being. Extra warning is better than confusion. If people feel strongly we can remove it later.

@nayeemrmn
Copy link
Collaborator

What do we do about the type declaration?

@bartlomieju
Copy link
Member

What do we do about the type declaration?

Ah, that's a good point. We don't have a way to add declarations conditionally (unless it's for the --unstable flag).

@ry
Copy link
Member Author

ry commented Jun 3, 2021

I think we'll just have to leave the type declaration incorrect when --location is not provided. The situation is not ideal. I don't want to make deno types conditional on --location too.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 3, 2021

We should amend the JSDoc to make it clear that --location is required to be set to make this available and potentially give an example. That way, people working on intelligent editors can discover it without having to run into the console message.

@kt3k
Copy link
Member

kt3k commented Jun 4, 2021

How about aligning the behavior with private mode / incognite mode of browsers? Provides localStorage object, but doesn't actually persist data. This doesn't break type declaration, and doesn't break feature detection code (though the result of feature detection might be wrong)

@caspervonb
Copy link
Contributor

Provides localStorage object, but doesn't actually persist data.

Is it no-ops, or is the localStorage effectively an instance of the session storage implementation?

@caspervonb
Copy link
Contributor

Is it no-ops, or is the localStorage effectively an instance of the session storage implementation?

To answer my own question, both.
Safari has a null storage implementation, chrome seems to use a temporary storage location.

@bartlomieju bartlomieju modified the milestones: 1.12.0, 1.13.0 Jul 7, 2021
@bartlomieju bartlomieju removed this from the 1.13.0 milestone Aug 10, 2021
@lucacasonato lucacasonato added this to the 1.14.0 milestone Aug 24, 2021
@bartlomieju bartlomieju modified the milestones: 1.14.0, future Sep 8, 2021
@CLAassistant
Copy link

CLAassistant commented Oct 15, 2021

CLA assistant check
All committers have signed the CLA.

@bartlomieju
Copy link
Member

This is stale. Closing without a merge.

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.

Feature detection typeof localStorage trips up permissions, requires --location to even start
8 participants