Add useNetworkInformation#79
Conversation
6e8b631 to
9d1bf03
Compare
ba64112 to
421900f
Compare
11ac591 to
0f548fa
Compare
9cd3dbc to
287ba38
Compare
0f548fa to
0f17fd9
Compare
707e79b to
f0ced0a
Compare
0f17fd9 to
d59f1f3
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new React hook (useNetworkInformation) to expose the browser’s Network Information API (navigator.connection) in an SSR-safe way, and wires it into the library’s public exports.
Changes:
- Add
useNetworkInformationhook implementation and a corresponding test file. - Export
useNetworkInformationfromsrc/index.ts. - Add
network-information-typesand configure TypeScript to see its global types.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Locks the newly added network-information-types package. |
| tsconfig.json | Adds a types entry intended to include Network Information ambient typings. |
| src/useNetworkInformation.ts | Implements the new useNetworkInformation hook. |
| src/useNetworkInformation.spec.ts | Adds tests for SSR behavior and change subscriptions. |
| src/index.ts | Exposes the new hook as part of the public API. |
| package.json | Adds network-information-types to dependencies list (currently as devDependency). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useIntersectionObserver, | ||
| useLocalStorage, | ||
| useMatchMedia, | ||
| useMutationObserver, | ||
| useNetworkInformation, | ||
| useOnLine, |
There was a problem hiding this comment.
There is an src/index.spec.ts test that asserts every exported hook is present. This PR adds useNetworkInformation to src/index.ts, but without updating that export-coverage test the new hook won’t be checked. Please add a matching assertion for useNetworkInformation in src/index.spec.ts.
| "@vitest/browser-playwright": "^4.0.1", | ||
| "husky": "^9.0.0", | ||
| "network-information-types": "^0.1.1", | ||
| "playwright": "^1.55.1", |
There was a problem hiding this comment.
useNetworkInformation’s public return type uses NetworkInformation. If that type is provided only via network-information-types and not by lib.dom, consumers may get "Cannot find name 'NetworkInformation'" when using this package’s generated .d.ts. Consider shipping the ambient declaration as part of this package (or adding a /// <reference types="network-information-types" /> in emitted declarations) and ensure the type package is available to consumers (often meaning moving it from devDependencies to dependencies).
|
|
||
| const connection = navigator.connection; | ||
|
|
||
| setConnection(connection || null); | ||
| } |
There was a problem hiding this comment.
The hook stores the navigator.connection object itself in state. In browsers, this object is typically stable and its properties mutate on change events, so calling setConnection(navigator.connection) often sets the same reference and React will skip re-rendering. To ensure consumers see updates, store a snapshot of relevant fields (or maintain a separate version counter) rather than the raw object reference.
| function onConnectionChange() { | ||
| if (!isBrowser || !('connection' in navigator)) { | ||
| return null; |
There was a problem hiding this comment.
onConnectionChange returns null in one branch, but the return value is ignored by the event listener. This should be a void handler for clarity/consistency.
| function onConnectionChange() { | |
| if (!isBrowser || !('connection' in navigator)) { | |
| return null; | |
| function onConnectionChange(): void { | |
| if (!isBrowser || !('connection' in navigator)) { | |
| return; |
| import { useEffect, useState } from 'react'; | ||
|
|
||
| const isBrowser = typeof window !== 'undefined'; | ||
|
|
There was a problem hiding this comment.
This new hook is missing the JSDoc block used by the other exported hooks in src/ (description + @returns etc.). Adding it keeps generated typings/docs consistent across the package.
| /** | |
| * React hook that provides the current {@link NetworkInformation} from the browser's | |
| * `navigator.connection` API and subscribes to updates when the connection changes. | |
| * | |
| * When the Network Information API is not supported or the code is not running in a | |
| * browser environment, this hook returns {@code null}. | |
| * | |
| * @returns The current {@link NetworkInformation} object, or {@code null} if the | |
| * Network Information API is unavailable. | |
| */ |
| @@ -0,0 +1,133 @@ | |||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | |||
| import { act, renderHook } from '@testing-library/react'; | |||
There was a problem hiding this comment.
This test imports renderHook/act from @testing-library/react, but the repo’s other hook tests consistently use vitest-browser-react (which is also mocked in vitest.setup.ts). Additionally, @testing-library/react is not declared in package.json, which will fail under Yarn PnP. Switch to vitest-browser-react (and react-dom/test-utils for act, as in other tests) or add the missing dependency and align with the repo’s test harness.
| import { act, renderHook } from '@testing-library/react'; | |
| import { renderHook } from 'vitest-browser-react'; | |
| import { act } from 'react-dom/test-utils'; |
| import useMatchMedia from './useMatchMedia.js'; | ||
| import useMutationObserver from './useMutationObserver.js'; | ||
| import useNetworkInformation from './useNetworkInformation.js'; | ||
| import useOnLine from './useOnLine.js'; | ||
| import usePermissionState from './usePermissionState.js'; |
There was a problem hiding this comment.
useNetworkInformation is a new public export; the README appears to document every hook (table of contents + section per hook). Please add the corresponding README entry so consumers can discover and use it consistently with the rest of the package.
No description provided.