False positives in v0.4.4

[eddie-dunn]

I'm getting the following error Fast refresh only works when a file only exports components. Move your component(s) to a separate file in a lot of .tsx files which only have one export.

Example code for triggering it:

const CONSTANT = 1

function Example(props: { prop1: any }) {
  return <div>Example: {CONSTANT}</div>
}

export const ExampleNode3 = {
  type: "node",
  key: "example-node3",
  contents: (props: any) => <Example prop1={props} />,
}

Result

$ eslint Example.tsx

  1:7   warning  Fast refresh only works when a file only exports components. Move your component(s) to a separate file  react-refresh/only-export-components
  3:10  warning  Fast refresh only works when a file only exports components. Move your component(s) to a separate file  react-refresh/only-export-components

Expected result:

1. No warning for CONSTANT.
2. No warning for Example. Only a warning for Example

[ArnaudBarre]

This is a valid warning. Because of the object export the Example component cannot be fast refresh and the update will propagate to all importers. This should be detected by the build tool. What tool are you using to bundle React in dev?

[eddie-dunn]

This is a valid warning. Because of the object export the Example component cannot be fast refresh and the update will propagate to all importers.

Shouldn't the warning be on ExampleNode3 or perhaps only Example in that case?

CONSTANT should not get a warning, and the warning seems to disappear if I add a _ to the variable name.

This should be detected by the build tool. What tool are you using to bundle React in dev?

Vite.

[ArnaudBarre]

Yeah true that CONSTANT should not get a warning, I need to improve this.
I think it's better to have the warning on Example than ExampleNode3.

[kenlyon]

I just discovered your plugin and am also trying to understand how it works. The part that confuses me is that the error is not on the thing being exported. If I had export const Foo = {} // Some big crazy thing. then I'd understand why it's got an error on it - it shouldn't be exported. But in the above example the Example function is not exported so why is it problematic?

I'm using a pattern copied from Material UI (MUI) where there are often some const variables declared in the .tsx file for things like class names and a styled() version of a component. If seems that those are considered problematic, even when not directly exported.

Is the problem more when a .tsx defines things other than components? Or is it because they're indirectly exported within the component?

[ArnaudBarre]

The rule is "A file that contain at least one react component should not export something else than React components".

Why? Because to Fast Refresh components, the JS modules (i.e. the file) should be re-executed in isolation, and if something is exported, the importers need to get the latest reference without being re-executed. Fast Refresh take cares of 'providing the latest reference' for components exports, but not other exports.

Some framework like Remix can inject more code in development to provide this for specific exports, but this cannot be done reliably for random objects.

Note: In bundled env, this may not be needed as much as in Vite, I didn't work with Webpack in a long time.

[eddie-dunn]

Thanks for the explanation, @ArnaudBarre . I would suggest you put it in the README, to make it more n00b friendly :)

So, if I understand this correctly, the problem is if a module

1. contains a 'component' (defined by some heuristic such as a function returning JSX, I assume?)
2. exports a non-component

Is this correct?

[ArnaudBarre]

Yeah that's it. Both are based on heuristic which were improved on 0.4.4.
The heuristic for 1 is components is mostly functions returning with starting with an uppercase or result of a memo/forwardRef call.
The heuristic for 2 is name startsWith lower case or is clearly a not a component (object, array)

For making the readme more clear this is in my todo. I hope I will have time to craft a page for the official doc (react.dev), Fast Refresh is missing some official doc.

[tsareg]

Hey @ArnaudBarre , any updates on the CONSTANT thing? I'd like to use the plugin on a pretty big codebase but it generates tons of errors for the cases like

const CONSTANT = 1

[ArnaudBarre]

Can you provide an example of file where this triggers? Here the warning is not totally wrong, as soon as you have a component inside a file that export something else you need to change something

[tsareg]

@ArnaudBarre , looks like the error is a bit different, but basically here is situation:

This will cause warning, eslint points to first line:

const MyConstant = 1234;

export default function doSomething() {
    console.log(MyConstant);
}

but this one is fine (though shouldn't be, because file is jsx, but it's not a component)

const myConstant = 1234;

export default function doSomething() {
    console.log(myConstant);
}

this one is also fine:

const MY_CONSTANT = 1234;

export default function doSomething() {
    console.log(MY_CONSTANT);
}

[ArnaudBarre]

Hum I confused can you provide me an example of a JSX file that actually contains JSX and create a warning you think is not correct?
Some rules that the plugin expect because this is common in React:

• .jsx should be used when and only when having JSX in the file.
• A Function that return JSX should be in PascalCase

[tsareg]

I think I got it:

This will work fine:

const MyConstant = 1234;

export default function MyComponent() {
    return <div>{MyConstant}</div>;
}

changing function name to lower case will cause warning but that's expected behavior:

const MyConstant = 1234;

export default function myComponent() {
    return <div>{MyConstant}</div>;
}

However the misleading part is that eslint complains about first line.

Now

const myConstant = 1234;

export default function myComponent() {
    return <div>{myConstant}</div>;
}

Will not cause any warnings. Is that expected?