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

WIP: perf(router): avoid calling set when the generated data unmodified #5482

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yoshinorin
Copy link
Member

@yoshinorin yoshinorin commented Apr 28, 2024

What does it do?

When any file is modified, even unchanged routes are updated (the set method is called). Additionally, in the set method, EventEmitter's emit is called. So, events to be fired many times (number of assets) whenever a single file is updated.

Maybe, it should be fine to call the set method only for the modified files.

Screenshots

N/A

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

Copy link

How to test

git clone -b perf/router-refresh https://github.com/hexojs/hexo.git
cd hexo
npm install
npm test

@yoshinorin
Copy link
Member Author

yoshinorin commented Apr 28, 2024

The test cases for the watcher are failing.

Now green. Use unique files for each test for watch. Because using the same files (test.txt) as other tests caused failures. Perhaps caused by caching or something similar, but I'm not sure.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8870147837

Details

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.978%

Totals Coverage Status
Change from base Build 8800035659: 0.0%
Covered Lines: 9297
Relevant Lines: 9393

💛 - Coveralls

@yoshinorin yoshinorin marked this pull request as ready for review April 28, 2024 20:22
@yoshinorin yoshinorin changed the title WIP: perf(router): avoid calling set when the generated data unmodified perf(router): avoid calling set when the generated data unmodified Apr 28, 2024
hexo.unwatch(); // Stop watching
await unlink(target); // Delete the file
}

it('watch() - source', async () => await testWatch(hexo.source_dir));
it('watch() - source', async () => await testWatch(hexo.source_dir, 'source.test.watch.txt'));
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to use unique files for each test for watch. Because using the same files (test.txt) as other tests caused failures. Perhaps caused by caching or something similar, but I'm not sure.

@yoshinorin yoshinorin changed the title perf(router): avoid calling set when the generated data unmodified WIP: perf(router): avoid calling set when the generated data unmodified Apr 28, 2024
@yoshinorin yoshinorin marked this pull request as draft April 28, 2024 21:26
@yoshinorin
Copy link
Member Author

yoshinorin commented Apr 28, 2024

I've noticed that when db.json exists before hexo s execution, CSS files and others aren't registered to the route in this PR. This is likely related to the tests failing when the file test.txt is used. I'll investigate.

@yoshinorin
Copy link
Member Author

I've noticed that when db.json exists before hexo s execution, CSS files and others aren't registered to the route in this PR. This is likely related to the tests failing when the file test.txt is used. I'll investigate.

It seems that the _routerRefresh method is called not only for updating the router but also for initializing the router. Therefore, it seems necessary to make changes such as separating the method for initialization and updating.

Comment on lines +629 to +630
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Always prefer // @ts-expect-error, do not use // @ts-ignore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants