This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Update Vue to v2.7 #1635
Merged
Update Vue to v2.7 #1635
Conversation
This file contains 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
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1635 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
openverse-bot
added
the
🚦 status: awaiting triage
|
Size Change: -10.3 kB (-1%) Total Size: 767 kB
ℹ️ View Unchanged
|
obulat
commented
Aug 11, 2022
package.json
Outdated
| "eslint-plugin-vuejs-accessibility": "^1.1.1", | ||
| "husky": "^7.0.1", | ||
| "jest": "^26.6.3", | ||
| "jest-transform-stub": "^2.0.0", | ||
| "lint-staged": "^12.3.7", | ||
| "module-alias": "^2.2.2", | ||
| "npm-run-all": "^4.1.5", | ||
| "pinia": "^2.0.11", |
There was a problem hiding this comment.
We apparently had pinia both in dependencies and devDependencies, with somewhat different versions specified.
obulat
commented
Aug 11, 2022
test/playwright/utils/breakpoints.ts
Outdated
| return expect( | ||
| await screenshotAble.screenshot(options) | ||
| ).toMatchSnapshot({ | ||
| test.describe(`screen at breakpoint ${breakpoint} with width ${screenWidth}`, () => { |
There was a problem hiding this comment.
For some reason, this was re-formatted by the linter. The main change in this file is the type of ExpectSnapshot, which was throwing an error.
obulat
force-pushed
the
update/vue
branch
3 times, most recently
from
9380070
to
c0a567d
Compare
obulat
added
💻 aspect: code
obulat
force-pushed
the
update/vue
branch
3 times, most recently
from
dc7ce73
to
d174ac2
Compare
7 tasks
obulat
commented
Aug 23, 2022
| @@ -70,13 +70,13 @@ export default defineComponent({ | |||
| </script> | |||
|
|
|||
| <style scoped> | |||
| ::v-deep .attribution { | |||
| ::v-deep(.attribution) { | |||
There was a problem hiding this comment.
The warning in #1634 starts appearing with the update to Vue 2.7, so I just fixed it here.
Closed
1 task
|
I managed to fix the playwright issue in #1646 (comment). One line fix to be able to update Playwright and avoid having to pin it. |
src/composables/use-focus-on-show.js
Outdated
| ensureFocus(dialog, { preventScroll: true, isActive }) | ||
| if (dialog.tabIndex === undefined || dialog.tabIndex < 0) { | ||
| warn(noFocusableElementWarning) | ||
| Vue.nextTick(() => { |
There was a problem hiding this comment.
nextTick is available from the composition API. What is the reason to pull it off of Vue directly rather than importing it from @nuxtjs/composition-api?
There was a problem hiding this comment.
The main advantage of importing it from Vue is that when we (hopefully, some time in the future) move to Nuxt3, we won't need to update the imports. Other than that, there is no reason, really. Do you think it makes sense to use @nuxtjs/composition-api? At least for consistency with the current state of the code?
There was a problem hiding this comment.
I think for consistency it makes sense. When (if) we move away from it, it will mostly be a find-and-replace job to update that import, right?
src/stores/feature-flag.ts
Outdated
Comment on lines
109
to
114
| for (const flagEntry of Object.entries(this.flags)) { | ||
| const [name, flag] = flagEntry as [string, FeatureFlag] | ||
| if (getFlagStatus(flag) === SWITCHABLE) { | ||
| flag.preferredState = cookies[name] | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this a change to appease TypeScript?
I don't remember if this is still the case, but at one point using a for...of loop actually incurred a non-trivial size penalty due to the generator runtime transpilation of it. Might not be an issue any more though.
There was a problem hiding this comment.
Yes, it's TS appeasing change. I didn't know about for ... of performance. I think I can improve this code by typing this
typings/csstype/index.d.ts
Outdated
| @@ -1,3 +1,4 @@ | |||
| import type * as CSS from 'csstype' // eslint-disable-line @typescript-eslint/no-unused-vars | |||
There was a problem hiding this comment.
What is this import for?
There was a problem hiding this comment.
There was an error without it, I'll have to look it up at my computer tomorrow. It's also how module augmentation is done in the csstype docs (the third point here): https://github.com/frenic/csstype#what-should-i-do-when-i-get-type-errors
There was a problem hiding this comment.
Gotcha. Can you add a link to the docs you shared here?
There was a problem hiding this comment.
vue-tsc gives the following error when we don't have the import:
src/components/VAudioTrack/layouts/VBoxLayout.vue(2,18): error TS2322: Type '{ width: string | undefined; }' is not assignable to type 'StyleValue | undefined'.
Object literal may only specify known properties, and 'width' does not exist in type 'CSSProperties | StyleValue[]'.
src/components/VSkeleton/VAudioTrackSkeleton.vue(16,23): error TS2322: Type '{ width: string; }' is not assignable to type 'StyleValue | undefined'.
Object literal may only specify known properties, and 'width' does not exist in type 'CSSProperties | StyleValue[]'.
src/components/VAudioTrack/VWaveform.vue(94,17): error TS2322: Type '{ width: string; left: string; }' is not assignable to type 'StyleValue | undefined'.
Object literal may only specify known properties, and 'width' does not exist in type 'CSSProperties | StyleValue[]'.
I guess without the import it considers that CSSProperties type only has the properties that are in the typings/csstype/index.d.ts instead of augmenting it...
obulat
force-pushed
the
update/vue
branch
2 times, most recently
from
d34f706
to
a2a0450
Compare
|
It looks like even with the pinned Playwright version there are still lots of visual regression tests 😕 |
|
I think I've found the issue. The dev version was working fine, but when you ran |
obulat
commented
Aug 25, 2022
test/playwright/utils/navigation.ts
Outdated
| @@ -248,7 +248,7 @@ export const goToSearchTerm = async ( | |||
| page.waitForNavigation(), | |||
| page.click(`[aria-label="${t('search.search', dir)}"]`), | |||
| ]) | |||
| await page.waitForLoadState('networkidle') | |||
| await page.waitForLoadState('domcontentloaded') | |||
There was a problem hiding this comment.
This makes the tests faster.
There was a problem hiding this comment.
Much faster indeed. I'm not sure 100% we should use domcontentloaded over load because load also downloads stylesheets and images.
There was a problem hiding this comment.
You are right, we should use load to make sure that all the images are there for the snapshots. Thank you!
dhruvkb
approved these changes
Aug 26, 2022
obulat
commented
Aug 26, 2022
obulat
commented
Aug 26, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
This PR updates Vue to version 2.7 which backports the composition API to Vue 2. This should (hopefully) make the future conversion to Nuxt 3 easier.
The main updated packages are Vue (to version 2.7) and
@nuxtjs/composition-apiwhich supports Vue 2.7 (0.33.1).Other changes:
vue-loaderadded to devDependencies to force the ^15.10 version as described in Vue 2.7 update post.Problems that I encountered:
node_modules/.bindirectory and symlinks no longer being created after upgrading from v6 to v8 npm/cli#4308)Testing Instructions
The CI tests should pass. The app should behave as normal everywhere.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin