fix(vue-renderer): read target from class instead of serverContext
#7752
+1
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `renderRoute` method assumes that `this.serverContext.nuxt.options` is a valid path, but that does not seem like a good assumption AFAICT. We could defensively check for its existence, but I think the correct thing to do here, is to use `this.options` instead.
|
I had to amend the author on my commit as I am not representing my employer with this change :) |
Rather than using `this.serverContext.options.target`, we should make use of the shortcut `this.options.target`.
Codecov Report
@@ Coverage Diff @@
## dev #7752 +/- ##
=======================================
Coverage 68.83% 68.83%
=======================================
Files 90 90
Lines 3821 3821
Branches 1034 1034
=======================================
Hits 2630 2630
Misses 968 968
Partials 223 223
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Hi, @timrchavez thanks for the PR and well-detailed explanations ❤️ It totally makes sense to merge this PR as both a fix and simplification refactor. BTW I also opened pimlie/nuxt-lambda#6 to fix the root cause. |
pi0
changed the title
target from class instead of serverContext
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
renderRoutemethod assumes thatthis.serverContext.nuxt.optionsis a valid path, but that does not seem like a good assumption AFAICT. We could defensively check for its existence, but I think the correct thing to do here, is to usethis.optionsinstead.I'm not sure how this was working before, but I hit this particular problem while trying to use
nuxt-lambdawhich creates a minimalserverContextthat doesn't includeserverContext.nuxt.options. See: https://github.com/pimlie/nuxt-lambda/blob/master/src/renderer.js#L8-L21 -- I freely admit that the bug could lie there. I tried to find some authoratative documentation onserverContextbut was unsuccessful.Types of changes
Description
Presently
renderRouterassumes thatserverContexthas anuxt.options.targetfield. This seems at odds with other areas within the code that need to accesstargetand get it directly fromserverContext.options. To remain consistent and remove (what I think is) an errant path, we should update the assignment ofserverContext.targetusingthis.options.targetinstead ofthis.serverContext.nuxt.options.targetChecklist:
I tried to run the tests locally but failed hard. Could not get jest to work.