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

refactor: [M3-8762] - only-export-components for Tanstack routes #11142

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Oct 22, 2024

Description 📝

This PR only intends to avoid degradation of the developer experience with with HMR invalidates and fast refresh, specifically (now) for the routes, considering how high in the tree they are. It does nothing to functionality or UI, just prevents potential future regressions with fast refresh when the Tanstack Router migration is well under.

Screenshot 2024-10-23 at 16 17 57
source

We sometimes get warnings related to this in the console when touching some files while developing. The linting will now provide insight when this is happening in our files, and the fixing can be done on a per case basis. They may not always be important to fix, but a good reminder to separate concerns and keep React happy and doing its thing.

Screenshot 2024-10-23 at 16 19 12

Usually the resolution is to avoid combining Component and function exports within the same file by extracting either into its own file. It can also sometimes be related to wrong capping of your function/component.

Note

  • If you edit a file that only exports React component(s), Fast Refresh will update the code only for that file, and re-render your component. You can edit anything in that file, including styles, rendering logic, event handlers, or effects.
  • If you edit a file with exports that aren’t React components, Fast Refresh will re-run both that file, and the other files importing it. So if both Button.js and Modal.js import theme.js, editing theme.js will update both components.
  • Finally, if you edit a file that’s imported by files outside of the React tree, Fast Refresh will fall back to doing a full reload. You might have a file which renders a React component but also exports a value that is imported by a non-React component. For example, maybe your component also exports a constant, and a non-React utility file imports it. In that case, consider migrating the constant to a separate file and importing it into both files. This will re-enable Fast Refresh to work. Other cases can usually be solved in a similar way.

Here is the gist, taken from the eslint rule page:

  • export * are not supported and will be reported as a warning
  • Anonymous function are not supported (i.e export default function() {})
  • Class components are not supported
  • All-uppercase function export is considered an error when not using direct named export
    ( ex const CMS = () => <></>; export { CMS } )

Changes 🔄

  • Implement new only-export-components eslint rule
  • Fix all warnings for Tanstack routes

How to test 🧪

Verification steps

I frankly am not too sure how to repro the fast refresh errors in the console consistently, so the verification steps here should be making sure that there's no regression in the application. This PR is more preventive than anything else.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@abailly-akamai abailly-akamai marked this pull request as ready for review October 23, 2024 20:32
@abailly-akamai abailly-akamai requested a review from a team as a code owner October 23, 2024 20:32
@abailly-akamai abailly-akamai requested review from dwiley-akamai and bnussman-akamai and removed request for a team October 23, 2024 20:32
Copy link

Coverage Report:
Base Coverage: 86.99%
Current Coverage: 86.99%

@bnussman-akamai bnussman-akamai changed the title refactor: [M3-8762] only-export-components for Tanstack routes refactor: [M3-8762] - only-export-components for Tanstack routes Oct 24, 2024
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

No adverse impacts observed in Cloud ✅

@abailly-akamai abailly-akamai merged commit 82f3542 into linode:develop Oct 25, 2024
23 checks passed
Copy link

cypress bot commented Oct 25, 2024

Cloud Manager E2E    Run #6732

Run Properties:  status check passed Passed #6732  •  git commit 82f3542ba2: refactor: [M3-8762] - `only-export-components` for Tanstack routes (#11142)
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #6732
Run duration 28m 02s
Commit git commit 82f3542ba2: refactor: [M3-8762] - `only-export-components` for Tanstack routes (#11142)
Committer Alban Bailly
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 445
View all changes introduced in this branch ↗︎

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.

3 participants