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

useStyles enhancement #198

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

useStyles enhancement #198

wants to merge 3 commits into from
+3 −3

Conversation

jameswhf
Copy link

No description provided.

@DenysIvko
Copy link

@dazlious hello there! Any progress on this PR?
This isBrowser check seems to always return undefined. If you take a look at dist/useStyles you'll see that this is transpiled into var _this = undefined and is never assigned with actual value:

// version 5.3.2
var _this = undefined;

var isBrowser = function () {
  // _this is undefined, so isBrowser() always returns undefined 
  return _this && typeof _this.window === 'object';
}();

https://github.com/kriasoft/isomorphic-style-loader/pull/198/files#diff-78c41e68a949b765a2709713f56f0f5e6542ffe54315f44719e44c413c0fc2e2L16

I would really appreciate if you could merge this PR since it fixes issue described above

dazlious
dazlious reviewed Jan 18, 2022
View reviewed changes
src/useStyles.js
import StyleContext from './StyleContext'

// To detect if it's in SSR process or in browser. Wrapping with
// the function makes rollup's replacement of "this" avoidable
// eslint-disable-next-line func-names
const isBrowser = (() => this && typeof this.window === 'object')()
const isBrowser = (() => typeof window === 'object')()
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

a more typical way of checking would be:

typeof window !== 'undefined'

src/useStyles.js
@@ -25,7 +25,7 @@ function useStyles(...styles) {
}
}
if (isBrowser) {
useEffect(runEffect, [])
useLayoutEffect(runEffect, styles)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what are you trying to solve here?
I know this repo does not have any tests, but we should start adding them in. Would be nice if you can provide some for your special usecase

@dazlious
Copy link
Collaborator

dazlious commented Jul 9, 2024

@dazlious hello there! Any progress on this PR? This isBrowser check seems to always return undefined. If you take a look at dist/useStyles you'll see that this is transpiled into var _this = undefined and is never assigned with actual value:

// version 5.3.2
var _this = undefined;

var isBrowser = function () {
  // _this is undefined, so isBrowser() always returns undefined 
  return _this && typeof _this.window === 'object';
}();

https://github.com/kriasoft/isomorphic-style-loader/pull/198/files#diff-78c41e68a949b765a2709713f56f0f5e6542ffe54315f44719e44c413c0fc2e2L16

I would really appreciate if you could merge this PR since it fixes issue described above

Still waiting for a reply here: #198 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dazlious dazlious dazlious left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@jameswhf @DenysIvko @dazlious