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

fix: skip empty text instance creation in jsx conditions #1604

Merged
merged 6 commits into from Jul 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/mean-pots-allow.md
@@ -0,0 +1,5 @@
---
'@react-pdf/renderer': patch
---

fix: skip empty text instance creation in jsx conditions
27 changes: 20 additions & 7 deletions packages/renderer/src/renderer.js
Expand Up @@ -11,6 +11,23 @@ import propsEqual from './utils/propsEqual';

const emptyObject = {};

const appendChild = (parentInstance, child) => {
const isParentText = parentInstance.type === 'TEXT';
const isChildTextInstance = child.type === 'TEXT_INSTANCE';
const isOrphanTextInstance = isChildTextInstance && !isParentText;

// Ignore orphan text instances.
// Caused by cases such as <>{name && <Text>{name}</Text>}</>
if (isOrphanTextInstance) {
console.warn(
`Invalid '${child.value}' string child outside <Text> component`,
);
return;
Copy link

Choose a reason for hiding this comment

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

Hi, I think there are some mistakes here. If we haven't push these node, that may got error when insertBefore called.

    insertBefore(parentInstance, child, beforeChild) {
      const index = parentInstance.children?.indexOf(beforeChild);

      if (index === undefined) return;

      if (index !== -1 && child)
        parentInstance.children.splice(index, 0, child);
    },

child will be droped if beforeChild is not OrphanTextInstance

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you provide a test that shows the error?

Copy link

Choose a reason for hiding this comment

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

import { Document, Font, Image, Link, Page, PDFViewer, StyleSheet, Text, View } from '@react-pdf/renderer'
import ReactMarkdown from 'react-markdown'
import { ReactMarkdownProps } from 'react-markdown/lib/ast-to-react'

const App = () => (
  <PDFViewer style={{ width: '100%', height: 800 }}>
    <Document>
      <Page style={styles.body}>
        {/*<Text>*/}
        <ReactMarkdown components={{
          h1: ({ children }: ReactMarkdownProps) => <Text>{children}</Text>,
          p: ({ children }: ReactMarkdownProps) => <Text>{children}</Text>,
        }}>{`# a
b`}</ReactMarkdown>
        {/*</Text>*/}
      </Page>
    </Document>
  </PDFViewer>
)

image

if add <Text> out of ReactMarkdown:

image

Actually, That can be found without ReactMarkdown. But I don't know when will insertBefore be called.

}

parentInstance.children.push(child);
};

const createRenderer = ({ onChange = () => {} }) => {
return ReactFiberReconciler({
schedulePassiveEffects,
Expand All @@ -23,9 +40,7 @@ const createRenderer = ({ onChange = () => {} }) => {

warnsIfNotActing: false,

appendInitialChild(parentInstance, child) {
parentInstance.children.push(child);
},
appendInitialChild: appendChild,

createInstance(type, { style, children, ...props }) {
return {
Expand Down Expand Up @@ -83,15 +98,13 @@ const createRenderer = ({ onChange = () => {} }) => {

useSyncScheduling: true,

appendChild(parentInstance, child) {
parentInstance.children.push(child);
},
appendChild,

appendChildToContainer(parentInstance, child) {
if (parentInstance.type === 'ROOT') {
parentInstance.document = child;
} else {
parentInstance.children.push(child);
appendChild(parentInstance, child);
}
},

Expand Down
48 changes: 48 additions & 0 deletions packages/renderer/tests/orphanTexts.test.js
@@ -0,0 +1,48 @@
/* eslint-disable react/jsx-curly-brace-presence */
import React from 'react';
import { Text, Document, Page } from '@react-pdf/primitives';
import renderToImage from './renderComponent';

const emptyString = '';

const mount = async children => {
const image = await renderToImage(
<Document>
<Page>{children}</Page>
</Document>,
);

return image;
};

describe('renderer', () => {
test('empty string', async () => {
const image = await mount(<>{emptyString && <Text>{emptyString}</Text>}</>);

expect(image).toMatchImageSnapshot();
});

test('string', async () => {
const image = await mount(<>{'text' || <Text>text</Text>}</>);

expect(image).toMatchImageSnapshot();
});

test('boolean', async () => {
const image = await mount(<>{true || <Text>text</Text>}</>);

expect(image).toMatchImageSnapshot();
});

test('zero', async () => {
const image = await mount(<>{0 && <Text>text</Text>}</>);

expect(image).toMatchImageSnapshot();
});

test('numbers', async () => {
const image = await mount(<>{10 || <Text>text</Text>}</>);

expect(image).toMatchImageSnapshot();
});
});
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.