-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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(vue-renderer): split renderer into ssr, spa and modern #5559
Conversation
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.
Really nice refactors @clarkdo ⚡ My comments are further than the refactor but can be cool if we can have them :)
@pi0 Refactored as code review comments |
} | ||
|
||
createRenderer() { | ||
throw new Error('`createRenderer()` needs to be implemented') |
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.
This has no coverage as we are internally always implementing these functions. Is it necessary to keep? @clarkdo
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.
At least keep the function signature for consistent common definition?
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.
JS is not a typed language to expect defining interface. Maybe can be useful for typescript or ts typings. For coverage, we can either ignore the line or write extra internal tests :)
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.
Not for type, just for reminding contributors which are gonna implement new renderer.
Yes, could be moved to ts types.
Codecov Report
@@ Coverage Diff @@
## dev #5559 +/- ##
==========================================
- Coverage 95.68% 95.62% -0.06%
==========================================
Files 78 81 +3
Lines 2595 2608 +13
Branches 661 661
==========================================
+ Hits 2483 2494 +11
- Misses 93 95 +2
Partials 19 19
Continue to review full report at Codecov.
|
Types of changes
Description
Changes:
renderer
functionality intossr
,spa
andmodern
renderer,renderer
is for managing renderers, resources/template and ready state, the specificssr,spa,modern
renderer is for app rendering.req.modernMode
toreq._modern
for more explicit to indicate request statusrenderer
for getting correct modern mode in renderer initialization.Checklist: