-
Notifications
You must be signed in to change notification settings - Fork 52
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(react): rename useIsMount & update useEffect to useIsomorphicLayoutEffect #246
Conversation
…morphicLayoutEffect
🦋 Changeset detectedLatest commit: 8c72993 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov Report
@@ Coverage Diff @@
## main #246 +/- ##
=======================================
Coverage 98.63% 98.63%
=======================================
Files 25 25
Lines 878 879 +1
Branches 152 152
=======================================
+ Hits 866 867 +1
Misses 12 12
|
packages/react/src/Suspense.tsx
Outdated
@@ -21,10 +21,10 @@ if (process.env.NODE_ENV !== 'production') { | |||
DefaultSuspense.displayName = 'Suspense' | |||
} | |||
const CSROnlySuspense = (props: SuspenseProps) => { | |||
const isMounted = useIsMounted() | |||
const isClient = useIsCSROnly() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isClient = useIsCSROnly() | |
const isClient = useIsClient() |
Can we change the name of this hook to useIsClient? If this hook is likely to be reused elsewhere in the future, I don't think it will only be used in XXX.CSROnly. In that case, I think a better name would be useIsClient rather than useIsCSROnly. Looking at the code, the return value is also the variable name isClient, so I think useIsClient would be a better name. Because the name XXX.CSROnly also means Client Side Render Only, which can be contained by isClient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im down
Then our Suspense
Should it be Suspense.Client
, not Suspense.CSROnly
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okinawaa I think<Suspense.CSROnly/>
using useIsClient
is best case.
then we will not go through BREAKING CHANGE
and also naming this component as <Suspense.Client/>
is not more clear to express client side render only
than <Suspense.CSROnly/>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manudeli
I agree with you.
CSROnly naming will keep, and useIsCSROnly naming is will changed to useIsClient.
fix: convert naming useIsCSROnly to useIsClient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for accepting my opinion
@okinawaa @minsoo-web This is test using visualization Code// websites/visualization/src/app/react/CSROnly1/page.tsx
'use client'
import { Suspense } from '@suspensive/react'
import Link from 'next/link'
import React from 'react'
export default function Page() {
return (
<div>
<Suspense.CSROnly fallback={<div style={{ backgroundColor: 'red' }}>loading...</div>}>
<Custom/>
</Suspense.CSROnly>
</div>
)
}
const Custom = () => {
return (
<div>
CSROnly1
<Link href={'/react/CSROnly2'}>to CSROnly2</Link>
</div>
)
} // websites/visualization/src/app/react/CSROnly2/page.tsx
'use client'
import { Suspense } from '@suspensive/react'
import Link from 'next/link'
import React from 'react'
export default function Page() {
return (
<div>
<Suspense.CSROnly fallback={<div style={{ backgroundColor: 'red' }}>loading...</div>}>
<Custom/>
</Suspense.CSROnly>
</div>
)
}
const Custom = () => {
return (
<div>
CSROnly2
<Link href={'/react/CSROnly1'}>to CSROnly1</Link>
</div>
)
} AS-IS useEffect in CSROnlyIf no useIsomorphicLayoutEffect, We can check sometimes red loading... fallback on every time routing. TO-BE useIsomorphicLayoutEffect in CSROnlyNo red loading... fallback on every time. we can provide better UX. issue resolved |
@minsoo-web I think this is cool work! big thanks to you making issue and resolving it 👍👍👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool 👍
Overview
I update useIsMount hook's name to useIsClient
Although I need to think more about the name, I named this hook because it is not an externally exported package, but an embedded hook used to distinguish whether it is a client environment or not.
I update useIsClient hook inside of Suspense.CSROnly from useEffect to useIsomorphicLayoutEffect
Although it is used differently depending on where it is used,
useEffect
is called at the mount stage, butuseLayoutEffect
is executed synchronously immediately after rendering, so it is possible to check whether it is in a client state or not first, so this hook is used.PR Checklist