Skip to content

Remove compat build #875

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.

Sign up for GitHub

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

Merged
hasanakg merged 1 commit into from
Feb 5, 2024
Merged

Remove compat build #875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ module.exports = {
// "import Counter from '@/Counter.vue'"
// (no need for a full path)
"moduleNameMapper": {
"^vue$": "@vue/compat",
'^@vue/composition-api$': '@vue/compat',
'^@wmde/wikit-vue-components$':
'@wmde/wikit-vue-components/dist/wikit-vue-components-vue3compat.common.js',
'^wikit-dist(.*)$': "<rootDir>/node_modules/@wmde/wikit-vue-components/dist$1",
Copy link
Contributor

@chukarave chukarave Feb 5, 2024

Choose a reason for hiding this comment

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

I think it's also safe to remove lines 25 to 27

Copy link
Collaborator Author

@hasanakg hasanakg Feb 5, 2024

Choose a reason for hiding this comment

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

I think removing wikit is not in the scope of this ticket, that's why I left these lines.

Copy link
Contributor

@guergana guergana Feb 5, 2024

Choose a reason for hiding this comment

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

I think removing wikit is not in the scope of this ticket, that's why I left these lines.

@hasanakg You are right that this is not the scope of this ticket but i think there is no ticket to remove wikit yet, we should do a cleanup afterwards.

Expand All @@ -33,18 +31,5 @@ module.exports = {
// Further info: https://test-utils.vuejs.org/migration/#-vue-vue3-jest-jest-28
"testEnvironmentOptions": {
"customExportConditions": ["node", "node-addons"],
},
// For Vue migration build
// Add compat config to test as well
"globals": {
"vue-jest": {
"compilerOptions": {
compatConfig: {
MODE: 3,
COMPILER_V_ON_NATIVE: true,
COMPILER_V_BIND_SYNC: true
}
}
}
}
}
14 changes: 0 additions & 14 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
"dependencies": {
"@inertiajs/inertia": "^0.11.0",
"@inertiajs/inertia-vue3": "^0.6.0",
"@vue/compat": "^3.3.8",
"@wmde/wikit-tokens": "^2.1.0-alpha.15",
"@wmde/wikit-vue-components": "^2.1.0-alpha.16",
Copy link
Contributor

@chukarave chukarave Feb 5, 2024

Choose a reason for hiding this comment

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

I think we can remove lines 62 and 63 too

Copy link
Collaborator Author

@hasanakg hasanakg Feb 5, 2024

Choose a reason for hiding this comment

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

I think removing wikit is not in the scope of this ticket, that's why I left these lines.

"date-fns": "^3.3.1",
Expand Down
7 changes: 0 additions & 7 deletions resources/js/Components/ItemIdSearchTextarea.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,6 @@ import { useStore } from '../store';
import { CdxTextArea, CdxField, CdxProgressBar } from "@wikimedia/codex";
import ValidationError from '../types/ValidationError';

// Run it with compat mode
// https://v3-migration.vuejs.org/breaking-changes/v-model.html
CdxTextArea.compatConfig = {
...CdxTextArea.compatConfig,
COMPONENT_V_MODEL: false,
};

const validationError: Ref<ValidationError> = ref(null);

const messages = useI18n();
Expand Down
4 changes: 2 additions & 2 deletions resources/js/Components/LoadingOverlay.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import { ref } from 'vue';
import { CdxProgressBar } from "@wikimedia/codex";

const props = withDefaults(defineProps<{
delay: number
visible: boolean
delay?: number
visible?: boolean
Copy link
Contributor

@chukarave chukarave Feb 5, 2024

Choose a reason for hiding this comment

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

Isn't making these optional kind of risky? If we somehow don't pass visible the show() method breaks. Is there a way around this that leaves them required?

Copy link
Collaborator Author

@hasanakg hasanakg Feb 5, 2024

Choose a reason for hiding this comment

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

They have default values, therefore they won't be null or undefined

Copy link
Contributor

@guergana guergana Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

They have default values, therefore they won't be null or undefined

I also had this issue, i was getting a TS warning or error because of this at some point but i dont see it anymore and they do have defaults, but I have two questions:

  • is this part of the scope of this ticket? 😬 aren't we better off putting this in the cleanup or as separate small PR?
  • why did you decide to change them to optional?

Copy link
Collaborator Author

@hasanakg hasanakg Feb 5, 2024

Choose a reason for hiding this comment

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

Valid question :D Vue documentation says that you should remove all the warnings before removing the compat build, I took it as a sign :D

Copy link
Contributor

@guergana guergana Feb 5, 2024

Choose a reason for hiding this comment

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

Valid question :D Vue documentation says that you should remove all the warnings before removing the compat build, I took it as a sign :D

ah ok, so you got the warning.. i also saw it. yes, we shouldn't merge code with warnings :)

Copy link
Contributor

@guergana guergana Feb 5, 2024

Choose a reason for hiding this comment

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

Isn't making these optional kind of risky? If we somehow don't pass visible the show() method breaks. Is there a way around this that leaves them required?

@chukarave the only instance of the loading overlay we are using in the app is not passing the props and this is the reason we are getting the warnings <loading-overlay ref="overlayRef" /> https://github.com/wmde/wikidata-mismatch-finder/blob/development/resources/js/Pages/Results.vue#L3 so yeah, the right call is to make them optional, since they are not being passed anyway

Copy link
Contributor

@chukarave chukarave Feb 6, 2024

Choose a reason for hiding this comment

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

Right those were exactly the warnings we were getting. Nice to have a clean console 🙌 well done @hasanakg

}>(), {
delay: 250,
visible: false
Expand Down
6 changes: 0 additions & 6 deletions resources/js/Pages/Results.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,12 +199,6 @@ import type { Ref } from 'vue';
import { computed, onMounted, ref } from 'vue';
import axios from 'axios';

// Run it with compat mode
// https://v3-migration.vuejs.org/breaking-changes/v-model.html
CdxCheckbox.compatConfig = {
...CdxCheckbox.compatConfig,
COMPONENT_V_MODEL: false,
};
interface MismatchDecision {
id: number,
item_id: string,
Expand Down
9 changes: 0 additions & 9 deletions resources/js/shims-vue.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,6 @@ declare module "*.vue" {
export default component;
}

declare module 'vue' {
import { CompatVue } from '@vue/runtime-dom'
const Vue: CompatVue
export default Vue
export * from '@vue/runtime-dom'
const { configureCompat } = Vue
export { configureCompat }
}

declare module 'vue-banana-i18n';

declare module '@wikimedia/language-data';
Expand Down
2 changes: 1 addition & 1 deletion tests/Vue/Components/MismatchRow.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ describe('MismatchesRow.vue', () => {

await wrapper.find('.full-description-button').trigger('click');

const dialog = wrapper.find('.full-description-dialog .cdx-dialog');
Copy link
Contributor

@guergana guergana Feb 5, 2024

Choose a reason for hiding this comment

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

everything is working flawlessly, but how is this related to the compat build??? 🤔

Copy link
Collaborator Author

@hasanakg hasanakg Feb 5, 2024

Choose a reason for hiding this comment

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

TBH, I have no idea. The old one works with compat build, but needed to update to the new one to make it work without compat build.

Copy link
Contributor

@guergana guergana Feb 5, 2024

Choose a reason for hiding this comment

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

I think I had this issue as well where the wrapper element collapses with the child element. I understand, ok.

const dialog = wrapper.find('.full-description-dialog.cdx-dialog');

expect(dialog.isVisible()).toBe(true);
});
Expand Down
19 changes: 0 additions & 19 deletions webpack.mix.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,29 +15,10 @@ mix.ts('resources/js/app.ts', 'public/js')
.webpackConfig({
resolve: {
alias: {
vue: '@vue/compat',
'@vue/composition-api': '@vue/compat',
'@wmde/wikit-vue-components':
'@wmde/wikit-vue-components/dist/wikit-vue-components-vue3compat.common.js',
'wikit-dist': path.resolve(__dirname, './node_modules/@wmde/wikit-vue-components/dist'),
}
},
module: {
rules: [
{
test: /\.vue$/,
loader: 'vue-loader',
options: {
compilerOptions: {
compatConfig: {
MODE: 3,
COMPILER_V_ON_NATIVE: true,
COMPILER_V_BIND_SYNC: true
}
}
}
}
]
}
})
.vue({ version: 3})
Expand Down