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.

Sign up for GitHub

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

Jump to bottom

fix(file-router): avoid errors and full reloads from Vite HMR when a route is added or removed #3367

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(file-router): avoid errors and full reloads from Vite HMR when a route is added or removed #3367

wants to merge 1 commit into from

Conversation

platosha
Copy link
Contributor

Fixes #2552

@platosha platosha force-pushed the fix/ap/fs-route-removal branch from fb7dc53 to f8d4b34 Compare March 21, 2025 10:09
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
24.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@codecov Codecov
Copy link

codecov bot commented Mar 21, 2025

Codecov Report

Attention: Patch coverage is 86.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.95%. Comparing base (799fd0a) to head (f8d4b34).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/ts/file-router/src/vite-plugin.ts 85.71% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3367      +/-   ##
==========================================
+ Coverage   86.89%   86.95%   +0.05%     
==========================================
  Files         115      115              
  Lines        8289     8310      +21     
  Branches     1271     1283      +12     
==========================================
+ Hits         7203     7226      +23     
+ Misses       1072     1070       -2     
  Partials       14       14
Flag Coverage Δ
unittests 86.95% <86.66%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Lodin
Lodin requested changes Mar 24, 2025
View reviewed changes
Copy link
Contributor

@Lodin Lodin left a comment

Choose a reason for hiding this comment

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

Overall, PR looks good. I have only a couple of stylistic changes

packages/ts/file-router/src/vite-plugin.ts
// be considered within the HMR update that caused the routes update.

const mg = this.environment.moduleGraph;
const fileRoutesModules = mg.getModulesByFile(fileURLToPath(runtimeUrls.code).replaceAll('\\', '/'))!;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const fileRoutesModules = mg.getModulesByFile(fileURLToPath(runtimeUrls.code).replaceAll('\\', '/'))!;
const fileRoutesModules = mg.getModulesByFile(fileURLToPath(runtimeUrls.code, { windows: false }))!;

packages/ts/file-router/src/vite-plugin.ts
// Make eager update for file routes in Vite module graph
// for consistency with the generated file contents.
await Promise.all(
[...fileRoutesModules].map(async (fileRouteModule) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[...fileRoutesModules].map(async (fileRouteModule) => {
Array.from(fileRoutesModules, async (fileRouteModule) => {

packages/ts/file-router/src/vite-plugin.ts
configResolved({ logger, root, build: { outDir } }) {
const _root = pathToFileURL(root);
const _generatedDir = new URL(generatedDir, _root);

_viewsDir = new URL(viewsDir, _root);
_viewsDirUsingSlashes = fileURLToPath(_viewsDir, {}).replaceAll('\\', '/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_viewsDirUsingSlashes = fileURLToPath(_viewsDir, {}).replaceAll('\\', '/');
_viewsDirPosix = fileURLToPath(_viewsDir, { windows: false });

packages/ts/file-router/src/vite-plugin.ts
@@ -62,14 +62,18 @@ export default function vitePluginFileSystemRouter({
let _outDir: URL;
let _logger: Logger;
let runtimeUrls: RuntimeFileUrls;
let _generateRuntimeFiles: () => Promise<boolean>;
let _viewsDirUsingSlashes: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let _viewsDirUsingSlashes: string;
let _viewsDirPosix: string;

packages/ts/file-router/test/vite-plugin/vite-plugin.spec.ts
// Updates to file-routes.ts are not processed separately, instead they
// are combined with the update of the triggering file.
expect(mockEnvironment.hot.send).to.not.be.called;
expect(modulesToUpdate).to.be.deep.equal([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(modulesToUpdate).to.be.deep.equal([]);
expect(modulesToUpdate).to.be.an('array').that.is.empty;

packages/ts/file-router/test/vite-plugin/vite-plugin.spec.ts
// HMR for routes and view file together.
expect(modulesToUpdate).to.not.equal(undefined);
expect(modulesToUpdate!.length).to.equal(2);
expect(modulesToUpdate![0]).to.eq(fileRoutesModule);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use that weird chai's aliases 😓 I'm always confused about what they mean. If it is a deep equality, let's have to.be.deep.equal(), otherwise to.be.equal.

packages/ts/file-router/test/vite-plugin/vite-plugin.spec.ts
const modulesToUpdate = await hotUpdate({ type: 'delete', file: viewFilePath, modules: [viewFileModule] });
expect(mockEnvironment.fetchModule).to.be.calledWith(fileRoutesTsPath);
// HMR for only file routes.
expect(modulesToUpdate).to.not.equal(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and in other places:

Suggested change
expect(modulesToUpdate).to.not.equal(undefined);
expect(modulesToUpdate).to.not.be.undefined;

packages/ts/file-router/test/vite-plugin/vite-plugin.spec.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Since SonarCloud is upset about repeating code, I'd suggest to either put the repeated part into a separate describe block with custom beforeEach, or to put the repeating code into a function.

packages/ts/file-router/test/vite-plugin/vite-plugin.spec.ts
Comment on lines +200 to +202
expect(modulesToUpdate).to.not.equal(undefined);
expect(modulesToUpdate!.length).to.equal(1);
expect(modulesToUpdate![0]).to.eq(fileRoutesModule);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(modulesToUpdate).to.not.equal(undefined);
expect(modulesToUpdate!.length).to.equal(1);
expect(modulesToUpdate![0]).to.eq(fileRoutesModule);
expect(modulesToUpdate).to.be.deep.equal([fileRoutesModule]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lodin Lodin Lodin requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Errors when removing a route
2 participants
@platosha @Lodin