new rule: render-result-naming-convention

[nickmccurdy]

Inspiration: Using wrapper as the variable name for the return value from render

If possible, it would be nice to extend or reuse the existing core rule prefer-destructuring so it only applies to wrapper or return values of render.

[nickmccurdy]

@Belco90 I thought about this example from #165 (comment), but I don't think I agree with it:

const setUp = () => render(<Foo />);

const utils = setUp();

👆 this should throw an error

If you're using screen (which I'm recommending by default in #169 anyway), this is actually fine if you just want to simplify rendering the same thing in separate tests. If you're not using screen, this would still be a valid smoke test, and there's still no Enzyme-style wrapper object so I believe it's still following the suggestions in the article.

[nickmccurdy]

Another thing to consider is that the article alternatively suggests a view object that isn't destructured. Do we want to support this?

I don't really like it personally, especially since screen replaces most of the use of this anyway. I suppose there could be a separate prefer-view-over-wrapper rule but I'm not sure if there would be demand for it.

[Belco90]

This is what I mentioned in the original discussions. There are two different things involved here: forcing destructuring or prefer calling it view.

By the original discussion I thought you proposed just the first one, but actually you want both as per described in KCD post. This only reports if you keep render result in a var and it's not called view.

Should we rename the rule then? I think prefer-destructuring-render is fine if we make crystal clear in the rule docs that saving render returned value in a var named view is a valid exception.

Valid examples:

// destructuring directly from render
const { rerender } = render(<Foo />);

// saving render result in view
const view = render(<Foo />);

// destructuring from method wrapping render
const setUp = () => render(<Foo />);
const { rerender } = setUp();

// saving method wrapping render result in view
const setUp = () => render(<Foo />);
const view = setUp();

Invalid examples:

// saving render result in a var other than view
const wrapper = render(<Foo />);
const utils = render(<Bar />);

// saving method wrapping render result in a var other than view
const setUp = () => render(<Foo />);
const utils = setUp();
const wrapper = setUp();

[nickmccurdy]

The article seems to really prioritize destructuring over view. I would personally rather keep the implementation of this rule and the suggestions more focused and just require destructuring, unless there's anyone that feels strongly that view should still be supported. I'm curious what @kentcdodds thinks as well.

[Belco90]

I like to have the possibility to keep render returned values under view variable basically for creating a superset when:

• creating custom render utils, like renderWithRedux or renderWithTheme

function renderWithRedux(ui, reduxOptions = {}, renderFn = render) {
  const {
    initialState,
    store = createStore(reducer, initialState),
  } = reduxOptions;

  const view = {
    ...renderFn(<Provider store={store}>{ui}</Provider>),
    store,
  };

  view.rerenderWithRedux = (newUi) =>
    renderWithRedux(newUi, { store }, view.rerender);

  return view;
}

• extracting duplicated queries from my tests into a common setUp method

const setUp = () => {
    const view = render(<Foo />);

    const getSomethingDynamically = val => view.getByRole('button', { name: val });
	const getSomethingRepetitive = () => view.getByRole('button', { name: 'submit' };


    return { getSomethingDynamically, getSomethingRepetitive, ...view };

Both cases can be rewritten destructuring view but the code is easier to read and maintain like this I think. I could disable the rule in these specific lines tho.

[nickmccurdy]

Fair enough, those seem like valid use cases to me. However I'm not sure if they'd be common enough to warrant explicitly allowing them, vs just disabling them inline as you suggested. Do you think we could at least do an initial release that forces destructuring, or would it be too confusing? Another thing to consider is that if we end up changing our mind over time, the old rule name could get confusing and we may have to rename it (unless we choose a generic name in advance).

[kentcdodds]

I think the following should be allowed:

• destructuring
• view
• utils

[Belco90]

I actually use utils in some old projects 😅 . I'd go then for allowing those 3 variants, and choosing a more appropriate name for the rule maybe?

Suggestions for rule name? I think even prefer-destructuring-render can work, specifying those 2 valid exceptions.

Should the rule then fix the code if other than view or utils is used as a var name?

[timdeschryver]

Don't know if it's possible to provide a fix, you would also need to update all the references to it. AFAIK this isn't straight forward.

What about renderresult-naming-convention, similar to ts eslint's naming convention rule.

[Belco90]

@timdeschryver nice one! Let's skip the fix then and rename it to render-result-naming-convention.

[Belco90]

No one got this one assigned right? I'll start working on it.

[github-actions]

🎉 This issue has been resolved in version 4.0.0-beta.1 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[github-actions]

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀