Library incorrectly report no-node-access when referencing props.children

[Snapperfish]

Library complains Avoid direct Node access. Prefer using the methods from Testing Library. for the following code props.children:

I want to be able to insert children, or a default node.

return (
  <>
    { // Other elements here. }
    {'children' in props ? (
      props.children
    ) : (
      <MyComponent {...props} disabled={disabled} />
    )}
  </>
)

See https://stackoverflow.com/q/67099835/481207

[Belco90]

Hey @Snapperfish, thanks for reporting this issue!

For clarifying the issue: this happens on testing files too, so restricting the plugin to be run against test files only won't fix it.

I think we should treat this as a bug then, but not really sure how to prevent it. The rule shouldn't report node accesses if coming from function args? There are still some cases that should be reported:

// valid cases

function ComponentA(props) {
  // this should NOT be reported
  if (props.children) {
    // ...
  }

  // this should NOT be reported
  return <div>{props.children}</div>
}

function ComponentB({ children }) {
  // this should NOT be reported
  if (children) {
    // ...
  }

  // this should NOT be reported
  return <div>{children}</div>
}

invalid cases

function assertElement(element) {
  // this SHOULD be reported
  expect(element.children).toBeDefined()
}

function getParagraphs({ children }) {
  // this SHOULD be reported
  return children.closest('p')
}

test('some test', () => {
  const section = screen.getByRole('article')
  assertElement(section)
  const paragraphs = getParagraphs(section)
});

So I'm afraid just relying just on children coming from function args wouldn't be enough. I guess we can ignore those function named following PascalCase so we assume they are components?

[MichaelDeBoey]

I guess we can ignore those function named following PascalCase so we assume they are components?

That could already be a start I think.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Belco90]

I definitely need to avoid stalebot to close issues with bug label 😅

[titenis]

Hello, this is still an issue, even for simple objects that have children key. For example I have this object that I use in tests:

const navItemWithChildren = {
  title: 'Cars',
  children: [
    {
      title: 'Add car',
    },
    {
      title: 'All cars',
    },
  ],
};

and I use this object in assertion like this:

expect(
      screen.getByText(navItemWithChildren.children[0].title),
    ).toBeVisible();

I get eslint error 41:44 error Avoid direct Node access. Prefer using the methods from Testing Library testing-library/no-node-access This is totally a false positive

[JustFly1984]

I got same false negative in one of projects I work on. props.children should be valid. added ignore comment meanwhile.

[thepuzzlemaster]

I am running into the same issue in my test. I have a helper method which renders my component, of which i can pass in a partial props object to override any defaults the test is using. I am trying to reference the children property of a button element that is being created, but getting the warning, which seems unnecessary in this case.

I'm just trying to avoid hard-coding the children string in multiple places in my test file.

it(`'editDisabled' prop should disable the 'edit' and 'auxiliary' buttons`, () => {
    const auxiliaryProps = { children: 'auxButton' }
    setup(getEditableInput({ auxiliaryProps, editDisabled: true })) // Sets up some spies and calls render()
    const editButton = screen.getByRole('button', { name: 'edit' })
    // eslint-disable-next-line testing-library/no-node-access
    const auxButton = screen.getByRole('button', { name: auxiliaryProps.children })
    expect(editButton.getAttribute('disabled')).not.toBeNull()
    expect(auxButton.getAttribute('disabled')).not.toBeNull()
  })

[sterlingwalsh]

I have a data structure unrelated to any html element that just uses the word 'children' as a key on an object that I have a test for and this rule is being triggered.

[smmoosavi]

as a workaround you can use this code:

const {children} = props

[SevenOutman]

Any update on this issue? Same problem here when mocking components with jest.fn

jest.mock('@/features/auth', () => ({
  Gate: jest.fn((props) => props.children), // Got "Avoid direct Node access. Prefer using the methods from Testing Library."
}));

[scottstern]

Any update on this issue? Same problem here when mocking components with jest.fn

jest.mock('@/features/auth', () => ({
  Gate: jest.fn((props) => props.children), // Got "Avoid direct Node access. Prefer using the methods from Testing Library."
}));

Having the same issue. destructuring children worked, but surprised this errored out

[SevenOutman]

Any update on this issue? Same problem here when mocking components with jest.fn

jest.mock('@/features/auth', () => ({
  Gate: jest.fn((props) => props.children), // Got "Avoid direct Node access. Prefer using the methods from Testing Library."
}));

Having the same issue. destructuring children worked, but surprised this errored out

No luck here in my TypeScript project. Have to deal with props declaration if using destructuring. :(

[sjarva]

I have opened a PR (#658) that deals with the most common and asked for cases (asked by @Snapperfish, @JustFly1984, @SevenOutman and @scottstern).

Unfortunately, this PR does not fix the errors on a) destructured props (mentioned by @Belco90) ...

function ComponentB({ children }) {
  // this should NOT be reported
  if (children) {
    // ...
  }

  // this should NOT be reported
  return <div>{children}</div>
}

or b) an object that has a property named children (example from @titenis but @thepuzzlemaster and @sterlingwalsh had a similar cases)

const navItemWithChildren = {
  title: 'Cars',
  children: [
    {
      title: 'Add car',
    },
    {
      title: 'All cars',
    },
  ],
};

render(<NavItem props={navItemWithChildren} />);

expect(screen.getByText(navItemWithChildren.children[0].title),).toBeVisible();

I am somewhat sure that both of these remaining cases a) and b) can be resolved, but I don't have all this code figured out yet. I'll open new issue(s) about these when this issue is closed 📝

[Belco90]

I am somewhat sure that both of these remaining cases a) and b) can be resolved, but I don't have all this code figured out yet. I'll open new issue(s) about these when this issue is closed

Happy to take care of this in separate issues.

[github-actions[bot]]

:tada: This issue has been resolved in version 5.7.2 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[sjarva]

Unfortunately, this PR does not fix the errors on a) destructured props (mentioned by @Belco90) ...

  // this should NOT be reported
  if (children) {
    // ...
  }

  // this should NOT be reported
  return <div>{children}</div>
}

Turns out, that this code does not produce an error for this rule, so I haven't opened an issue for this, but added it to valid unit tests for this rule (in #684 ).

or b) an object that has a property named children (example from @titenis but @thepuzzlemaster and @sterlingwalsh had a similar cases)

I have opened issue #683 for this scenario.