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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(utils): update isClient to check document too #3329

Merged
merged 3 commits into from Aug 25, 2023
Merged

Conversation

brc-dd
Copy link
Contributor

@brc-dd brc-dd commented Aug 20, 2023

Description

The current isClient implementation checks whether window is defined or not. This breaks things when using SSR with Deno as window is defined there (https://deno.land/api@v1.36.1?s=Window)

Additional context


馃 Generated by Copilot at 0c222fe

Fixed a bug in isClient function that incorrectly detected browser environments. Changed the function to use document instead of window in packages/shared/utils/is.ts.

馃 Generated by Copilot at 0c222fe

  • Fix bug where isClient returns true in non-browser environments (link)

@brc-dd
Copy link
Contributor Author

brc-dd commented Aug 20, 2023

Also, the window parameter of functions doesn't respect undefined and the types don't allow null.

@antfu
Copy link
Member

antfu commented Aug 21, 2023

This would be a bit tricky, as document could be later overridden as well. What do you think we check window and document together (does not solve the issue but makes it align more with previous behavior)?

@brc-dd
Copy link
Contributor Author

brc-dd commented Aug 21, 2023

Yeah that would work too.

packages/shared/utils/is.ts Outdated Show resolved Hide resolved
Co-authored-by: Divyansh Singh <40380293+brc-dd@users.noreply.github.com>
@brc-dd brc-dd changed the title fix(utils): update isClient to check document fix(utils): update isClient to check document too Aug 21, 2023
@antfu antfu added this pull request to the merge queue Aug 25, 2023
Merged via the queue into vueuse:main with commit 786cbba Aug 25, 2023
4 checks passed
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.

None yet

2 participants