Skip to content

Commit

Permalink
Detect not memoized file and options props
Browse files Browse the repository at this point in the history
  • Loading branch information
wojtekmaj committed Jan 10, 2024
1 parent 1f3491a commit 290d179
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 1 deletion.
1 change: 1 addition & 0 deletions packages/react-pdf/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"license": "MIT",
"dependencies": {
"clsx": "^2.0.0",
"dequal": "^2.0.3",
"make-cancellable-promise": "^1.3.1",
"make-event-props": "^1.6.0",
"merge-refs": "^1.2.1",
Expand Down
88 changes: 88 additions & 0 deletions packages/react-pdf/src/Document.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -604,4 +604,92 @@ describe('Document', () => {

expect(onTouchStart).toHaveBeenCalled();
});

it('does not warn if file prop was memoized', () => {
const spy = vi.spyOn(global.console, 'error').mockImplementation(() => {
// Intentionally empty
});

const file = { data: pdfFile.arrayBuffer };

const { rerender } = render(<Document file={file} />);

rerender(<Document file={file} />);

expect(spy).not.toHaveBeenCalled();

vi.mocked(global.console.error).mockRestore();
});

it('warns if file prop was not memoized', () => {
const spy = vi.spyOn(global.console, 'error').mockImplementation(() => {
// Intentionally empty
});

const { rerender } = render(<Document file={{ data: pdfFile.arrayBuffer }} />);

rerender(<Document file={{ data: pdfFile.arrayBuffer }} />);

expect(spy).toHaveBeenCalledTimes(1);

vi.mocked(global.console.error).mockRestore();
});

it('does not warn if file prop was not memoized, but was changed', () => {
const spy = vi.spyOn(global.console, 'error').mockImplementation(() => {
// Intentionally empty
});

const { rerender } = render(<Document file={{ data: pdfFile.arrayBuffer }} />);

rerender(<Document file={{ data: pdfFile2.arrayBuffer }} />);

expect(spy).not.toHaveBeenCalled();

vi.mocked(global.console.error).mockRestore();
});

it('does not warn if options prop was memoized', () => {
const spy = vi.spyOn(global.console, 'error').mockImplementation(() => {
// Intentionally empty
});

const options = {};

const { rerender } = render(<Document file={pdfFile.blob} options={options} />);

rerender(<Document file={pdfFile.blob} options={options} />);

expect(spy).not.toHaveBeenCalled();

vi.mocked(global.console.error).mockRestore();
});

it('warns if options prop was not memoized', () => {
const spy = vi.spyOn(global.console, 'error').mockImplementation(() => {
// Intentionally empty
});

const { rerender } = render(<Document file={pdfFile.blob} options={{}} />);

rerender(<Document file={pdfFile.blob} options={{}} />);

expect(spy).toHaveBeenCalledTimes(1);

vi.mocked(global.console.error).mockRestore();
});

it('does not warn if options prop was not memoized, but was changed', () => {
const spy = vi.spyOn(global.console, 'error').mockImplementation(() => {
// Intentionally empty
});

const { rerender } = render(<Document file={pdfFile.blob} options={{}} />);

rerender(<Document file={pdfFile.blob} options={{ maxImageSize: 100 }} />);

expect(spy).not.toHaveBeenCalled();

vi.mocked(global.console.error).mockRestore();
});
});
37 changes: 36 additions & 1 deletion packages/react-pdf/src/Document.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import makeCancellable from 'make-cancellable-promise';
import clsx from 'clsx';
import invariant from 'tiny-invariant';
import warning from 'warning';
import { dequal } from 'dequal';
import pdfjs from './pdfjs.js';

import DocumentContext from './DocumentContext.js';
Expand Down Expand Up @@ -233,6 +234,14 @@ const defaultOnPassword: OnPassword = (callback, reason) => {
}
};

function isParameterObject(file: File): file is Source {
return (
typeof file === 'object' &&
file !== null &&
('data' in file || 'range' in file || 'url' in file)
);
}

/**
* Loads a document passed using `file` prop.
*/
Expand Down Expand Up @@ -271,6 +280,32 @@ const Document = forwardRef(function Document(

const pages = useRef<HTMLDivElement[]>([]);

const prevFile = useRef<File>();
const prevOptions = useRef<Options>();

useEffect(() => {
if (file && file !== prevFile.current && isParameterObject(file)) {
warning(
!dequal(file, prevFile.current),
`File prop passed to <Document /> changed, but it's equal to previous one. This might result in unnecessary reloads. Consider memoizing the value passed to "file" prop.`,
);

prevFile.current = file;
}
}, [file]);

// Detect non-memoized changes in options prop
useEffect(() => {
if (options && options !== prevOptions.current) {
warning(
!dequal(options, prevOptions.current),
`Options prop passed to <Document /> changed, but it's equal to previous one. This might result in unnecessary reloads. Consider memoizing the value passed to "options" prop.`,
);

prevOptions.current = options;
}
}, [options]);

const viewer = useRef({
// Handling jumping to internal links target
scrollPageIntoView: (args: ScrollPageIntoViewArgs) => {
Expand Down Expand Up @@ -385,7 +420,7 @@ const Document = forwardRef(function Document(
);

invariant(
'data' in file || 'range' in file || 'url' in file,
isParameterObject(file),
'Invalid parameter object: need either .data, .range or .url',
);

Expand Down
1 change: 1 addition & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4528,6 +4528,7 @@ __metadata:
canvas: "npm:^2.11.2"
clsx: "npm:^2.0.0"
cpy-cli: "npm:^5.0.0"
dequal: "npm:^2.0.3"
eslint: "npm:^8.26.0"
eslint-config-wojtekmaj: "npm:^0.9.0"
jsdom: "npm:^21.1.0"
Expand Down

0 comments on commit 290d179

Please sign in to comment.