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

Upgrade Vue.js CLI to 4.x #969

Merged
merged 9 commits into from
Apr 30, 2020
Merged

Upgrade Vue.js CLI to 4.x #969

merged 9 commits into from
Apr 30, 2020

Conversation

lfarrell
Copy link
Contributor

@lfarrell lfarrell changed the title Upgrade Vue.js to 4.x Upgrade Vue.js CLI to 4.x Apr 29, 2020
Copy link
Member

@bbpennel bbpennel left a comment

Choose a reason for hiding this comment

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

Looks good over all, I left two questions to help me understand a little better. Seems like the await next tick thing is the main change, which seems like a cool feature.

else
cat static/js/admin/vue-permissions-editor/dist/app.js >> static/js/vue-permissions.js
endif
cat static/js/admin/vue-permissions-editor/dist/js/app*js >> static/js/vue-permissions.js
Copy link
Member

Choose a reason for hiding this comment

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

Just checking, but is the case now that we don't need to change behaviors depending on the deploy type for the permissions editor js, but do still need to for the css?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, the JS files should be the same now across deployment types. In the dev build the css still get bundled into the JS files.

this.$router.push({ name: 'displayRecords', query: this.urlParams(update_params) });
this.$router.push({ name: 'displayRecords', query: this.urlParams(update_params) })
.catch((e) => {
if (e.name !== 'NavigationDuplicated') {
Copy link
Member

Choose a reason for hiding this comment

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

what triggers NavigationDuplicated errors? Something like clicking twice on a link?

Copy link
Contributor Author

@lfarrell lfarrell Apr 30, 2020

Choose a reason for hiding this comment

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

I think that or trying to redirect to the same url you're at. If I read the ticket mentioned in the commit correctly, it shouldn't give this error if you update parameters. It's possible that doesn't include parameters's specified via the ?. It might only refer to params in the route, like :uuid in our case. Not catching the error doesn't break anything, but it does throw a bunch of warnings in the tests.

@@ -9,7 +10,7 @@ localVue.use(VueRouter);

const updated_uuid = 'c03a4bd7-25f4-4a6c-a68d-fedc4251b680';
const title = 'Test Collection';
const response = `<table><tbody><tr><th>Creator</th> <td><p>Real Dean</p></td></tr></tbody></table>`;
const response = pretty(`<table><tbody><tr><th>Creator</th><td><p>Real Dean</p></td></tr></tbody></table>`);
Copy link
Member

Choose a reason for hiding this comment

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

Real dean is getting prettified!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, vue test utils changed how they return HTML. It used to have no spaces, but they switched to formatting the HTML with the pretty package before returning it.

@lfarrell
Copy link
Contributor Author

The big change was vue-test-utils stopped running actions within tests synchronously. Hence all the async awaits scattered around in the tests.

@bbpennel bbpennel merged commit df38161 into fcrepo4 Apr 30, 2020
@bbpennel bbpennel deleted the vue-cli-update branch April 30, 2020 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants