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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(hook/utils): add useIsomorphicLayoutEffect #239

Merged
merged 12 commits into from
Oct 20, 2023

Conversation

minsoo-web
Copy link
Member

@minsoo-web minsoo-web commented Oct 19, 2023

Overview

To distinguish whether it is a client or server environment from a hook or component using useEffect

I will change the useEffect used in places where the client environment and the server environment should be distinguished to the useIsomorphicLayoutEffect.

PR Checklist

  • I did below actions if need
  1. I read the Contributing Guide
  2. I added documents and tests.

@changeset-bot
Copy link

changeset-bot bot commented Oct 19, 2023

⚠️ No Changeset found

Latest commit: 5aa8c3e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Oct 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 20, 2023 0:00am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
visualization ⬜️ Ignored (Inspect) Visit Preview Oct 20, 2023 0:00am

@vercel vercel bot temporarily deployed to Preview – visualization October 19, 2023 09:03 Inactive
@minsoo-web minsoo-web force-pushed the feat/add-useIsomorphicLayoutEffect branch from 93977cb to 555f571 Compare October 19, 2023 09:04
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #239 (5aa8c3e) into main (8d80537) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #239   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        22    +1     
  Lines          846       850    +4     
  Branches       149       150    +1     
=========================================
+ Hits           846       850    +4     
Components Coverage Δ
@suspensive/react 100.00% <100.00%> (ø)
@suspensive/react-query ∅ <ø> (∅)

@vercel
Copy link

vercel bot commented Oct 19, 2023

Deployment failed with the following error:

Resource is limited - try again in 22 minutes (more than 100, code: "api-deployments-free-per-day").

@@ -0,0 +1,3 @@
export const isServer = () => {
return typeof window === 'undefined' && typeof global !== 'undefined'
Copy link
Member

@manudeli manudeli Oct 19, 2023

Choose a reason for hiding this comment

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

I don't know why we should check global. could you explain it please?

No global checking libraries references

Copy link
Member Author

@minsoo-web minsoo-web Oct 20, 2023

Choose a reason for hiding this comment

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

I think, if in node env or server env, this is global so this check is semantically equal to typeof window === 'undefined'

Copy link
Member Author

@minsoo-web minsoo-web Oct 20, 2023

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

I can accept semantical cause. but I think if we don't check global in this case, we can get better performance. because we are making most fundamental library so this isClient function will be recalled many time. so I don't want to give up our better performance if we can

I asked this to slash library too. toss/slash#352

Every libraries I checked don't check global to know it is server or client. so I'm very curious your and slash's intention

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. this function is called every time in bundler position.
So this expression called inline, rather than export.

Copy link
Member Author

Choose a reason for hiding this comment

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

chore: delete isClient, isServer function

I delete it !! many thanks 🚀

Copy link
Member

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, Let's improve this if we need to improve it after

packages/react/src/utils/index.ts Outdated Show resolved Hide resolved
packages/react/src/hooks/useIsomorphicLayoutEffect.ts Outdated Show resolved Hide resolved
@vercel
Copy link

vercel bot commented Oct 19, 2023

Deployment failed with the following error:

Resource is limited - try again in 29 minutes (more than 100, code: "api-deployments-free-per-day").

@vercel vercel bot temporarily deployed to Preview – visualization October 19, 2023 12:00 Inactive
@vercel vercel bot temporarily deployed to Preview – docs October 19, 2023 15:17 Inactive
@vercel vercel bot temporarily deployed to Preview – docs October 20, 2023 07:49 Inactive
@vercel vercel bot temporarily deployed to Preview – visualization October 20, 2023 07:50 Inactive
@vercel vercel bot temporarily deployed to Preview – docs October 20, 2023 08:06 Inactive
@vercel vercel bot temporarily deployed to Preview – visualization October 20, 2023 08:07 Inactive
@manudeli
Copy link
Member

manudeli commented Oct 20, 2023

I think you need to use corepack with pnpm to meet your pnpm version as packageManager field of package.json. check this doc https://pnpm.io/installation#using-corepack

This is also good for install script
https://github.com/TanStack/query/blob/main/package.json#L12

@vercel vercel bot temporarily deployed to Preview – docs October 20, 2023 11:51 Inactive
@vercel vercel bot temporarily deployed to Preview – visualization October 20, 2023 11:53 Inactive
@manudeli
Copy link
Member

I think you need to use corepack with pnpm to meet your pnpm version as packageManager field of package.json. check this doc https://pnpm.io/installation#using-corepack

This is also good for install script https://github.com/TanStack/query/blob/main/package.json#L12

I resolved this issue in 80cec91 please use corepack please. I think we need to add this in CONTRIBUTING.md

@vercel vercel bot temporarily deployed to Preview – docs October 20, 2023 12:00 Inactive
@manudeli manudeli merged commit 03992b5 into main Oct 20, 2023
15 checks passed
@manudeli manudeli deleted the feat/add-useIsomorphicLayoutEffect branch October 20, 2023 12:07
manudeli added a commit that referenced this pull request Aug 3, 2024
# Overview

<!--
    A clear and concise description of what this pr is about.
 -->

To distinguish whether it is a client or server environment from a hook
or component using `useEffect`

I will change the `useEffect` used in places where the client
environment and the server environment should be distinguished to the
`useIsomorphicLayoutEffect`.

## PR Checklist

- [x] I did below actions if need

1. I read the [Contributing
Guide](https://github.com/toss/slash/blob/main/.github/CONTRIBUTING.md)
2. I added documents and tests.

---------

Co-authored-by: Jonghyeon Ko <[email protected]>
manudeli pushed a commit that referenced this pull request Aug 3, 2024
# Overview

<!--
    A clear and concise description of what this pr is about.
 -->

To distinguish whether it is a client or server environment from a hook
or component using `useEffect`

I will change the `useEffect` used in places where the client
environment and the server environment should be distinguished to the
`useIsomorphicLayoutEffect`.

## PR Checklist

- [x] I did below actions if need

1. I read the [Contributing
Guide](https://github.com/toss/slash/blob/main/.github/CONTRIBUTING.md)
2. I added documents and tests.

---------
manudeli added a commit that referenced this pull request Aug 3, 2024
# Overview

<!--
    A clear and concise description of what this pr is about.
 -->

To distinguish whether it is a client or server environment from a hook
or component using `useEffect`

I will change the `useEffect` used in places where the client
environment and the server environment should be distinguished to the
`useIsomorphicLayoutEffect`.

## PR Checklist

- [x] I did below actions if need

1. I read the [Contributing
Guide](https://github.com/toss/slash/blob/main/.github/CONTRIBUTING.md)
2. I added documents and tests.

---------

Co-authored-by: Jonghyeon Ko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants