-
Notifications
You must be signed in to change notification settings - Fork 59
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
Update Vue to 3.3.4 #831
Update Vue to 3.3.4 #831
Conversation
For some reason, running `npm update vue` didn't have much of an effect. I suspect that has to do with the @vue/test-utils dependency on @vue/server-renderer, which requires an exactly matching version of vue. I temporarily uninstalled @vue/test-utils, and afterwards, `npm update vue` updated vue successfully. I then reinstalled @vue/test-utils and confirmed that it was using the latest versions of @vue/server-renderer and vue. I ran the following commands: npm uninstall @vue/test-utils npm update vue npm install -D @vue/test-utils npm ls vue I can see that @vue/compiler-sfc was updated as well.
const shadowRouterProps = { | ||
created() { | ||
// eslint-disable-next-line no-self-assign | ||
if (this.$router != null) this.$router = this.$router; |
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.
I think this works because $router
is a property on the prototype chain. By assigning it to this
, we give this
its own $router
property. The vm
property of the Vue Test Utils wrapper seems to provide access to an instance's own properties, but not global properties.
$route() { | ||
return this.$router.currentRoute.value; | ||
return this.$router != null ? this.$router.currentRoute.value : undefined; | ||
} |
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.
We don't assign $route
to this
like we do with $router
because the value of $route
can change (whenever there's a navigation and the route changes). Unlike $router
, $route
as a global property seems to be a getter: https://github.com/vuejs/router/blob/a1611b6099403cfe9539ee8d4e9308b3a6a0175c/packages/router/src/router.ts#L1211-L1215
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.
looks good
This PR updates Vue to the latest version. I had to wrangle
npm
a little to make the update happen, but I was able to do so: see the commit message for details.In #830, I thought I had tested my change against the latest Vue. However, while I had run
npm update vue
, I don't think I confirmed that the update was successful, so I actually wasn't running against an updated Vue. When I did manage to update Vue, I saw an error that seems related to the change in #830. Here's part of the error:I don't have a good explanation for why this test of
FieldKeyList
fails, but tests beforeFieldKeyList
pass. Maybe becauseFieldKeyList
rendersProjectSubmissionOptions
, which accesses$route
in its template? Could it be that$container
, which is a Vue Test Utils mock, isn't being injected until later in the component's lifecycle compared to before?Ultimately, I don't think it's very important that we understand this error. Instead of computing
$router
based on$container
, we can copy the global property to each component. That way, components won't ever be accessing$container
; only tests will. When I make that change, tests pass again.Before submitting this PR, please make sure you have:
npm run test
andnpm run lint
and confirmed all checks still pass OR confirm CircleCI build passes