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

Conversation

jeetiss
Copy link
Collaborator

@jeetiss jeetiss commented Nov 20, 2021

Fix #1602

@jeetiss jeetiss self-assigned this Dec 10, 2021
@diegomura diegomura force-pushed the fix-jsx-conditions branch 2 times, most recently from 1e7417f to 8c76b81 Compare July 4, 2022 04:51
@diegomura diegomura marked this pull request as ready for review July 4, 2022 04:51
@diegomura
Copy link
Owner

@jeetiss I resume this. I think throwing in these cases is too agressive. On this PR I just ignore these cases with a warning

@diegomura diegomura merged commit 7eefc33 into master Jul 4, 2022
@diegomura diegomura deleted the fix-jsx-conditions branch July 4, 2022 05:08
@github-actions github-actions bot mentioned this pull request Jul 4, 2022
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.

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.

error Cannot read properties of undefined (reading 'height') at setNodeHeight
3 participants