[Feature]: isClient

[ssi02014]

Package Scope

@suspensive/react

Description

I noticed that isClient is used inside useIsomorphicLayoutEffect.

Couldn't "isClient" be managed in @suspicious/react/utils? Additionally, I think isSever could be defined and utilized as well.

However, currently the only code that uses "isClient" is "useIsomorphicLayoutEffect". Whether this should be managed in "utils" depends on the maintainer's thoughts.

If you agree with the above, I can work on that and adding test code.

Please let me know what you think! 🙏

Possible Solution

export const isClient = typeof window !== 'undefined'

export const isServer = !isClient()

etc.

No response

[manudeli]

@ssi02014 After seeing this issue, I spent a lot of time thinking about where to place isClient. Also, I thought it was necessary to compare the previously thought methods of functionalizing, variableizing, or inlining isClient, and conducted a comparison of the three methods.

isClient comparison

I compared what files were included in .next/server when building websites/visualization as shown in the photo below. I compared @suspensive/react's isClient implementation in three situations: I expected useIsomorphicLayoutEffect to be assigned only as useEffect on the server, and useLayoutEffect to be removed from the server-side build file.

How to reproduce

1. update useIsomorphicLayoutEffect's isClient as inline / variable / function
2. pnpm build (this script will build packages and build websites/visualization with our packages built right now)
3. find useIsomorphicLayoutEffect's code in websites/visualization/.next/server/chunks/*

1. inline

import { useEffect, useLayoutEffect } from 'react'

export const useIsomorphicLayoutEffect = typeof window !== 'undefined' ? useLayoutEffect : useEffect

2. variable

import { useEffect, useLayoutEffect } from 'react'

const isClient = typeof window !== 'undefined'

export const useIsomorphicLayoutEffect = isClient ? useLayoutEffect : useEffect

3. function

import { useEffect, useLayoutEffect } from 'react'

const isClient = () => typeof window !== 'undefined'

export const useIsomorphicLayoutEffect = isClient ? useLayoutEffect : useEffect

Conclusion

I think we need to remove all variable for isClient and just inline should be used for it to meet our expectation of useIsomorphicLayoutEffect.

@ssi02014 Could you do this please? @minsoo-web I mentioned you because you made useIsomorphicLayoutEffect in our package. I will assign this issue to @ssi02014 to fix if he accept.

[ssi02014]

@manudeli I agree that it's also important that the build output fits our intentions.

However, wouldn't it be better for readability and understanding by others outside the organization to manage it as a meaningful variable or function?

I don't see much difference in behavior between all three, so it would be nice to see other aspects considered.

[minsoo-web]

@manudeli Thank you very much for checking the results through the actual build, not just thinking.

If the build result does not include a control statement, but only useEffect or useLayoutEffect remains and runs, I don't think there's any reason to disagree with declaring it inline.

But every time it's executed, the control statement has to be executed (although of course it's a very cheap control statement) and since the isClient function itself is not an expensive function, does it have a lot of influence on the performance difference? It seems that I naturally come to worry about it.

So the way I want to suggest, what if we make one wrapper function including the control statement?

The approximate interface is as follows.

function isomorphic<T, F>(serverAction: T, clientAction: F) {
  return typeof window !== "undefined" ? serverAction : clientAction;
}

// use case
const useIsomorphicEffect = isomorphic(useEffect, useLayoutEffect);

We can take care of readability, and we don't have to carry unnecessary control every time, do we? That's what I thought.

I'm not sure, but I think this is a compromise...? I think it can be.

[manudeli]

@minsoo-web I tried building server file with isomorphic function as you suggest. It was great trying but you can see unnecessary useIsomorphicLayoutEffect in server file too.

function isomorphic<T, F>(serverAction: T, clientAction: F) {
  return typeof window !== "undefined" ? serverAction : clientAction;
}

// use case
const useIsomorphicEffect = isomorphic(useEffect, useLayoutEffect);

I don't think this is readability improvement

I think readability can only be said to have improved if it ensures the same behavior. I think that if unnecessary code is included in the built file for the sake of readability, this does not improve readability but rather fails to produce the expected results.

@ssi02014 @minsoo-web I agree that adding the variable name or function name of isClient to typeof window !== 'undefined' can help with readability only for isClient itself. However, using isClient as a function or variable causes unwanted code to be included in the built server file compared to using it inline. (Example: unnecessary useLayoutEffect is included in the implementation of useIsomorphicLayoutEffect in the built next.js server file) I think this is not simply a problem of readability, but a different expected result. As a library developer, I believe that we should not insert code that does not produce the expected results into code that many people rely on, such as @suspensive/react, simply because of readability issues.

From another perspective, the name useIsomorphicLayout also captures the behavior of the hook well. However, if useLayoutEffect is included in a server file built using the isClient variable or function inside useIsomorphicLayout, I think it is not performing its role properly as isomorphic as its name suggests. So I would like to remove isClient and replace it with inline.

Conclusion

In useIsomorphicLayoutEffect of built server file, If we use useLayoutEffect because of isClient variable or function, I think it's not isomorphic anymore. only inline we can make it correctly, no using useLayoutEffect in useIsomorphicLayoutEffect of built server file

[ssi02014]

@manudeli I understand and sympathize with your comments! 👍

@minsoo-web Thanks for the nice comments! 👍

[minsoo-web]

I agree with you. @manudeli

Wouldn't it be the same for me to make a Wrapper function, and eventually make an unnecessary function? I was lost in the thought that.

From the standpoint of providing libraries, I think it is correct to adopt the method if it is possible to reduce the execution of unnecessary functions or control statements to users.

I will be able to upload the task of changing the isClient function to inline now.

[minsoo-web]

@ssi02014 And I would like to express my deep gratitude to you for providing this issue.