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

fix: define window.name #20804

Merged
merged 6 commits into from
Oct 8, 2023
Merged

fix: define window.name #20804

merged 6 commits into from
Oct 8, 2023

Conversation

bartlomieju
Copy link
Member

Closes #20750

This matches what browsers do: https://developer.mozilla.org/en-US/docs/Web/API/Window/name

In the future we might want to change the behavior to actually update
the process name, but that needs a bit of discussion regarding if
it needs a permission flag (that would make polyfiling process.title
setter really easy too).

dsherret
dsherret approved these changes Oct 7, 2023
View reviewed changes
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

cli/tests/unit/globals_test.ts Outdated Show resolved Hide resolved
@bartlomieju bartlomieju enabled auto-merge (squash) October 8, 2023 21:31
@bartlomieju bartlomieju merged commit dfc254c into denoland:main Oct 8, 2023
13 checks passed
lucacasonato
lucacasonato reviewed Oct 9, 2023
View reviewed changes
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.

Mh I actually really dislike this global because folks often call variables name, and now if they forget to declare them with let / const first, setting/ reading will still work without throwing.

@bartlomieju
Copy link
Member Author

@lucacasonato so what would you suggest to do here? I think name is the perfect place to implement process name setter as it already follows what browsers do.

@bartlomieju bartlomieju deleted the globalthis_name branch October 9, 2023 10:43
@dsherret
Copy link
Member

dsherret commented Oct 9, 2023 •
edited
Loading

@bartlomieju shouldn't we deprecate window then do nothing here? Deno doesn't have the concept of a window unlike browsers.

@bartlomieju
Copy link
Member Author

I'm still not a fan of deprecating window, but if we're gonna go that route then it probably makes sense. Should I revert this PR?

@dsherret
Copy link
Member

dsherret commented Oct 9, 2023

My guess would be that the source of the original issue is that the window global exists in the first place.

bartlomieju added a commit that referenced this pull request Oct 12, 2023
@bartlomieju
fix: define window.name (#20804)
86722da 
Closes #20750

This matches what browsers do:
https://developer.mozilla.org/en-US/docs/Web/API/Window/name

In the future we might want to change the behavior to actually update
the process name, but that needs a bit of discussion regarding if
it needs a permission flag (that would make polyfiling `process.title`
setter really easy too).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucacasonato lucacasonato lucacasonato left review comments

@dsherret dsherret dsherret approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add global name
3 participants
@bartlomieju @dsherret @lucacasonato