-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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(server): exclude dist files request from browser detection #5571
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #5571 +/- ##
==========================================
+ Coverage 95.63% 95.64% +<.01%
==========================================
Files 81 81
Lines 2613 2615 +2
Branches 663 664 +1
==========================================
+ Hits 2499 2501 +2
Misses 95 95
Partials 19 19
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## dev #5571 +/- ##
===========================================
+ Coverage 51.12% 95.64% +44.51%
===========================================
Files 81 81
Lines 2615 2617 +2
Branches 664 665 +1
===========================================
+ Hits 1337 2503 +1166
+ Misses 988 95 -893
+ Partials 290 19 -271
Continue to review full report at Codecov.
|
@@ -158,6 +159,13 @@ export default class Server { | |||
} | |||
} | |||
|
|||
if (!this.options.dev) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this logic out of middleware stack and into modern renderer? (Check right before rendering)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still should be executed before devMiddleware in dev mode. And the chosen renderer depends on the request mode modern/legacy.
so if we move it, we can only move it to renderer instead of modern renderer and also find a way of sharing it(move to @nuxt/utils ) in builder/server for devMiddleware as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right. Having util would be also a nice idea to be shared between devMiddleware and renderer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We can introduce utils refactor in later PRs as well :)
Great, I’ll propose a pr later for extracting the sharing logic. |
Types of changes
Description
Checklist: