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

Unmount check #433

Merged
merged 2 commits into from
Jun 8, 2020
Merged

Unmount check #433

merged 2 commits into from
Jun 8, 2020

Conversation

promer94
Copy link
Collaborator

@promer94 promer94 commented Jun 7, 2020

No description provided.

@promer94
Copy link
Collaborator Author

promer94 commented Jun 7, 2020

Currently, the pre-commit hooks script is broken

tsc --noEmit
node_modules/jest-diff/build/diffLines.d.ts:8:13 - error TS1005: '=' expected.

8 import type { DiffOptions } from './types';
              ~

node_modules/jest-diff/build/diffLines.d.ts:8:34 - error TS1005: ';' expected.

8 import type { DiffOptions } from './types';
                                   ~~~~~~~~~

node_modules/jest-diff/build/index.d.ts:10:13 - error TS1005: '=' expected.

10 import type { DiffOptions } from './types';
               ~

node_modules/jest-diff/build/index.d.ts:10:34 - error TS1005: ';' expected.

10 import type { DiffOptions } from './types';
                                    ~~~~~~~~~

node_modules/jest-diff/build/index.d.ts:11:1 - error TS1128: Declaration or statement expected.

11 export type { DiffOptions, DiffOptionsColor } from './types';
   ~~~~~~

node_modules/jest-diff/build/index.d.ts:11:13 - error TS1005: ';' expected.

11 export type { DiffOptions, DiffOptionsColor } from './types';
               ~

node_modules/jest-diff/build/index.d.ts:11:52 - error TS1005: ';' expected.

11 export type { DiffOptions, DiffOptionsColor } from './types';
                                                      ~~~~~~~~~

node_modules/jest-diff/build/printDiffs.d.ts:8:13 - error TS1005: '=' expected.

8 import type { DiffOptions, DiffOptionsNormalized } from './types';
              ~

node_modules/jest-diff/build/printDiffs.d.ts:8:57 - error TS1005: ';' expected.

8 import type { DiffOptions, DiffOptionsNormalized } from './types';
                                                          ~~~~~~~~~
Found 9 errors.

need to upgrade Typescript to support import type { xxx } from 'xxx'

@promer94 promer94 force-pushed the unmount-check branch 2 times, most recently from 730315a to 6589e6a Compare June 7, 2020 09:44
Copy link
Member

@shuding shuding left a comment

Choose a reason for hiding this comment

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

Thank you @promer94 for working on this!

I think it's a good chance to refactor these handlers to event emitters:
callbacksRef.current.onLoadingSlow(...args)eventsRef.current.emit('onLoadingSlow', ...args)

So we don't need to implement every method inside callbacksRef, and it will be more flexible in the future.

eventsRef = useRef({
  emit: (event, ...args) => {
    if (unmounted) return
    config[event](...args)
  }
})

refactor handlers to event emitters
do unmount check for all events
Closes: vercel#286
@promer94 promer94 requested a review from shuding June 7, 2020 11:47
Copy link
Member

@shuding shuding left a comment

Choose a reason for hiding this comment

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

Code looks good, thank you @promer94 (also appreciate the tests)!

@shuding shuding merged commit dd533c0 into vercel:master Jun 8, 2020
@promer94 promer94 deleted the unmount-check branch June 9, 2020 12:34
@@ -215,6 +215,18 @@ function useSWR<Data = any, Error = any>(
const unmountedRef = useRef(false)
const keyRef = useRef(key)

// do unmount check for callbacks
const eventsRef = useRef({
Copy link

Choose a reason for hiding this comment

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

why not use useMemo in here, no reference scene

Copy link
Member

Choose a reason for hiding this comment

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

@thoamsy useMemo is more like to use for recalculation when deps changed.
for the events trigger, the refer here is just creating an object which is never changed during renders, without any deps required. so I feel ref is more proper for this case : )

promer94 added a commit to promer94/swr that referenced this pull request Apr 16, 2022
promer94 added a commit to promer94/swr that referenced this pull request Apr 17, 2022
promer94 added a commit to promer94/swr that referenced this pull request Apr 20, 2022
shuding pushed a commit that referenced this pull request Apr 20, 2022
himanshiLt pushed a commit to himanshiLt/swr that referenced this pull request Apr 26, 2022
shuding pushed a commit that referenced this pull request May 13, 2022
* type: extends useConfig cache interface (#1938)

* remove mount check for react18 (#1927)

reactwg/react-18#82

related pr #787 #433

* lint: dont check unused vars with underscore prefix (#1939)

* test: upgrade to jest 28 (#1942)

* Upgrade to jest 28

* Upgrade to jest 28

* feat: useSyncExternalStoreWithSelector

* refactor: remove stateUpdate and boardcast

state update should be handled by uSESW

* type: fix test type error

* remove pnpm.lock

* fix: import cjs for codesanbox

* refactor: add selector

* refactor: add cachestate interface and try fix custom cache

* fix: custom cache init

* refactor: remove useless flag

* chore: codesanbox ci

* refactor: remove force render in infinite

* build: add _internal

* chore: mark warning test

* fix: dts generation

* codesanbox ci

* chore: rename swr folder to core

Co-authored-by: Jiachi Liu <inbox@huozhi.im>
promer94 added a commit to promer94/swr that referenced this pull request May 13, 2022
* type: extends useConfig cache interface (vercel#1938)

* remove mount check for react18 (vercel#1927)

reactwg/react-18#82

related pr vercel#787 vercel#433

* lint: dont check unused vars with underscore prefix (vercel#1939)

* test: upgrade to jest 28 (vercel#1942)

* Upgrade to jest 28

* Upgrade to jest 28

* feat: useSyncExternalStoreWithSelector

* refactor: remove stateUpdate and boardcast

state update should be handled by uSESW

* type: fix test type error

* remove pnpm.lock

* fix: import cjs for codesanbox

* refactor: add selector

* refactor: add cachestate interface and try fix custom cache

* fix: custom cache init

* refactor: remove useless flag

* chore: codesanbox ci

* refactor: remove force render in infinite

* build: add _internal

* chore: mark warning test

* fix: dts generation

* codesanbox ci

* chore: rename swr folder to core

Co-authored-by: Jiachi Liu <inbox@huozhi.im>
shuding added a commit that referenced this pull request May 13, 2022
* update state in a transition

* support snapshotting external state

* wip

* move types:check to pre-commit

* refactor: switch to useSyncExternalStoreWithSelector (#1953)

* type: extends useConfig cache interface (#1938)

* remove mount check for react18 (#1927)

reactwg/react-18#82

related pr #787 #433

* lint: dont check unused vars with underscore prefix (#1939)

* test: upgrade to jest 28 (#1942)

* Upgrade to jest 28

* Upgrade to jest 28

* feat: useSyncExternalStoreWithSelector

* refactor: remove stateUpdate and boardcast

state update should be handled by uSESW

* type: fix test type error

* remove pnpm.lock

* fix: import cjs for codesanbox

* refactor: add selector

* refactor: add cachestate interface and try fix custom cache

* fix: custom cache init

* refactor: remove useless flag

* chore: codesanbox ci

* refactor: remove force render in infinite

* build: add _internal

* chore: mark warning test

* fix: dts generation

* codesanbox ci

* chore: rename swr folder to core

Co-authored-by: Jiachi Liu <inbox@huozhi.im>

* chore: useSES should be external (#1960)

* rename type

* types: update cache types

* chore: move @types/useSES to devDependencies

Co-authored-by: Shu Ding <g@shud.in>
Co-authored-by: Jiachi Liu <inbox@huozhi.im>
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.

4 participants