feat: Replace Vue.extend with defineComponent in design system (no-changelog) #5918

alexgrozav · 2023-04-06T10:54:44Z

Github issue / Community forum post (link here to close automatically):

github-actions · 2023-04-06T10:55:12Z

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

If fixing bug, added test to cover scenario.
If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

Added unit tests to cover new or updated functionality.

Files matching **/*.vue:

Used composition API for all new components.
Added component or unit tests to cover functionality.

Files matching packages/design-system/**/*.vue:

Used design system tokens (colors, spacings...) where possible.
Updated Storybook with new component or updated functionality.

Make sure to check off this list before asking for review.

codecov · 2023-04-06T11:09:06Z

Codecov Report

Patch coverage: 5.93% and project coverage change: +0.15 🎉

Comparison is base (3fdc441) 17.55% compared to head (9431c63) 17.70%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5918      +/-   ##
==========================================
+ Coverage   17.55%   17.70%   +0.15%     
==========================================
  Files        2498     2520      +22     
  Lines      114353   114668     +315     
  Branches    17866    17869       +3     
==========================================
+ Hits        20074    20302     +228     
- Misses      93687    93771      +84     
- Partials      592      595       +3

Impacted Files	Coverage Δ
...n-system/src/components/N8nFormInput/validators.ts	`0.00% <0.00%> (ø)`
...ackages/design-system/src/plugins/n8nComponents.ts	`0.00% <0.00%> (ø)`
packages/design-system/src/types/form.ts	`0.00% <0.00%> (ø)`
packages/design-system/src/types/index.ts	`0.00% <0.00%> (ø)`
packages/design-system/src/types/router.ts	`0.00% <0.00%> (ø)`
packages/design-system/src/types/user.ts	`0.00% <0.00%> (ø)`
packages/design-system/src/utils/event-bus.ts	`100.00% <100.00%> (ø)`

... and 65 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ivov

typecheck returns Found 141 errors in 44 files on this branch but Found 94 errors in 36 files on master. Is this expected?

ivov · 2023-04-06T13:35:58Z

packages/design-system/src/components/N8nMarkdown/Markdown.vue

 	methods: {
-		onClick(event: MouseEvent) {
+		onClick(event: MouseEvent): void {


I wouldn't add it when inferrable.

LGTM! All very minor comments so feel free to ignore.

The increase in typecheck errors is expected due to two reasons

Would it make sense to ignore type errors on spec files temporarily and add typecheck to any regular checks we do on design-system? I fixed all the type errors a while ago - it'd be nice to prevent them from accumulating again.

Added a task for fixing errors and adding typecheck to CI, but will likely be done after we update testing-library: https://linear.app/n8n/issue/N8N-6318/fix-design-system-typecheck-issues-and-add-typecheck-to-ci

ivov · 2023-04-06T13:37:12Z

packages/design-system/src/components/N8nUserSelect/UserSelect.vue

@@ -30,16 +30,17 @@
 </template>

 <script lang="ts">
-import 'vue';
-import mixins from 'vue-typed-mixins';
+/* eslint-disable vue/no-unused-components */


We're not using ElSelect and ElOption though.

ivov · 2023-04-06T13:42:14Z

packages/design-system/src/components/N8nUserSelect/UserSelect.vue

@@ -87,7 +86,7 @@ export default mixins(Locale).extend({
 					return false;
 				}

-				if (this.ignoreIds && this.ignoreIds.includes(user.id)) {
+				if ((this.ignoreIds as string[])?.includes(user.id)) {


Why is inference not working here? And if we're asserting it's string[], then ? should not be needed.

ivov · 2023-04-06T13:43:36Z

packages/design-system/src/plugins/n8nComponents.ts

@@ -1,4 +1,4 @@
-import Vue from 'vue';
+import { PluginObject } from 'vue';


Suggested change

import { PluginObject } from 'vue';

import type { PluginObject } from 'vue';

ivov · 2023-04-06T13:43:46Z

packages/design-system/src/plugins/n8nComponents.ts

@@ -97,4 +97,4 @@ export default {
 		app.component('n8n-resize-wrapper', N8nResizeWrapper);
 		app.component('n8n-recycle-scroller', N8nRecycleScroller);
 	},
-};
+} as PluginObject<{}>;


Can we type this instead of asserting?

ivov · 2023-04-06T13:44:25Z

packages/design-system/src/shims-markdown-it.d.ts

@@ -0,0 +1,4 @@
+declare module 'markdown-it-task-lists' {
+	import { PluginSimple } from 'markdown-it';


Suggested change

import { PluginSimple } from 'markdown-it';

import type { PluginSimple } from 'markdown-it';

ivov · 2023-04-06T13:49:39Z

packages/design-system/src/components/N8nFormInputs/FormInputs.vue

-			default() {
-				return [[]];
-			},
+			default: () => [[]],


Re: this comment, FormBox, UserSelect, DraggableTarget, ResourceLocator still use method syntax for array default.

Maybe we should update all the type: Array with a prop type assertion. Or later if too many.

alexgrozav · 2023-04-11T08:47:55Z

The increase in typecheck errors is expected due to two reasons:

the .spec files. The version of Vue Test Utils we're using does not support DefineComponent as a type, but will run tests all the same
the defineComponent directive provides better type checking

I've gone through most of the non-spec files and fixed the types. Can you check the changes out?

ivov

LGTM! All very minor comments so feel free to ignore.

The increase in typecheck errors is expected due to two reasons

Would it make sense to ignore type errors on spec files temporarily and add typecheck to any regular checks we do on design-system? I fixed all the type errors a while ago - it'd be nice to prevent them from accumulating again.

ivov · 2023-04-11T09:04:41Z

packages/design-system/src/components/N8nMenu/Menu.vue

@@ -56,7 +56,7 @@ import { Menu as ElMenu } from 'element-ui';
 import N8nMenuItem from '../N8nMenuItem';

 import { defineComponent, PropType } from 'vue';
-import { IMenuItem } from '../../types';
+import { IMenuItem, RouteObject } from '../../types';


type import

ivov · 2023-04-11T09:05:11Z

packages/design-system/src/components/N8nMenuItem/MenuItem.vue

@@ -74,7 +74,7 @@
 import { Submenu as ElSubmenu, MenuItem as ElMenuItem } from 'element-ui';
 import N8nTooltip from '../N8nTooltip';
 import N8nIcon from '../N8nIcon';
-import { IMenuItem } from '../../types';
+import { IMenuItem, RouteObject } from '../../types';


type import

ivov · 2023-04-11T09:05:50Z

packages/design-system/src/components/N8nRadioButtons/RadioButtons.vue

-import { defineComponent } from 'vue';
+import { defineComponent, PropType } from 'vue';
+
+export interface IRadioOption {


I think we want to move away from the I- prefix.

ivov · 2023-04-11T09:08:14Z

packages/design-system/src/styleguide/ColorCircles.vue

 			required: true,
 		},
 	},
 	created() {
 		const setColors = () => {
-			(this.colors as string[]).forEach((color: string) => {
+			this.colors.forEach((color: string) => {


string is inferrable here

ivov · 2023-04-11T09:08:48Z

packages/design-system/src/styleguide/VariableTable.vue

 		},
 	},
 	created() {
 		const setValues = () => {
-			(this.variables as string[]).forEach((variable: string) => {
+			this.variables.forEach((variable: string) => {


string is inferrable here

ivov · 2023-04-11T14:41:56Z

FYI to automate type imports: #5951

github-actions · 2023-04-12T14:22:06Z

✅ All Cypress E2E specs passed

* master: (62 commits) fix(editor): Redirect to home page after saving data on SAML onboarding page (no-changelog) (#5961) feat: Replace Vue.extend with defineComponent in design system (no-changelog) (#5918) feat(MySQL Node): Overhaul fix(OpenAI Node): Update models to only show those supported (#5805) ci: Add test for wait node (no-changelog) (#5414) fix(Github Trigger Node): Remove content_reference event (#5830) ci: Validate load options methods in nodes-base (no-changelog) (#5862) ci: Use `--chown=node:node` in COPY commands in the custom docker image (no-changelog) (#5913) 🚀 Release 0.224.0 (#5957) fix(NocoDB Node): Fix for updating or deleting rows with not default primary keys fix(HTTP Request Node): Show detailed error message in the UI again (#5959) ci: Prevent skipping of E2E fail job (no-changelog) (#5958) ci: Fix E2E tests on master (no-changelog) (#5960) refactor(core): Use injectable classes for db repositories (part-1) (no-changelog) (#5953) fix(core): Validate customData keys and values (#5920) (no-changelog) feat(editor): Add user activation survey (#5677) fix(editor): Update vite legacy-plugin browser target (no-changelog) (#5952) docs: Fix typo in AWS S3 and S3 nodes for parent folder key (#5933) fix(core): Update xml2js to address CVE-2023-0842 (#5948) fix(Code Node): Update vm2 to address CVE-2023-29017 (#5947) ... # Conflicts: # packages/workflow/src/Interfaces.ts

…rce-mapper-ui * feature/resource-mapping-component: (62 commits) fix(editor): Redirect to home page after saving data on SAML onboarding page (no-changelog) (#5961) feat: Replace Vue.extend with defineComponent in design system (no-changelog) (#5918) feat(MySQL Node): Overhaul fix(OpenAI Node): Update models to only show those supported (#5805) ci: Add test for wait node (no-changelog) (#5414) fix(Github Trigger Node): Remove content_reference event (#5830) ci: Validate load options methods in nodes-base (no-changelog) (#5862) ci: Use `--chown=node:node` in COPY commands in the custom docker image (no-changelog) (#5913) 🚀 Release 0.224.0 (#5957) fix(NocoDB Node): Fix for updating or deleting rows with not default primary keys fix(HTTP Request Node): Show detailed error message in the UI again (#5959) ci: Prevent skipping of E2E fail job (no-changelog) (#5958) ci: Fix E2E tests on master (no-changelog) (#5960) refactor(core): Use injectable classes for db repositories (part-1) (no-changelog) (#5953) fix(core): Validate customData keys and values (#5920) (no-changelog) feat(editor): Add user activation survey (#5677) fix(editor): Update vite legacy-plugin browser target (no-changelog) (#5952) docs: Fix typo in AWS S3 and S3 nodes for parent folder key (#5933) fix(core): Update xml2js to address CVE-2023-0842 (#5948) fix(Code Node): Update vm2 to address CVE-2023-29017 (#5947) ... # Conflicts: # packages/workflow/src/Interfaces.ts

…angelog) (n8n-io#5918) * refactor: replace new Vue() with custom event bus (no-changelog) * fix: export types from design system main * fix: update component types * fix: update form inputs event bus * refactor: replace global Vue references in design-system * refactor: update prop types * feat: improve types * fix: further type improvements * fix: further types improvements * fix: further type improvements * test: fix test snapshots * test: fix snapshot * chore: fix linting issues * test: fix personalization modal snapshot

alexgrozav added 7 commits March 24, 2023 17:54

refactor: replace new Vue() with custom event bus (no-changelog)

e816419

fix: export types from design system main

d8c0976

Merge branch 'master' into n8n-6269-replace-vue-event-buses-with-custom

c2bb7a0

fix: update component types

f6a9071

fix: update form inputs event bus

39f285d

Merge branch 'master' into n8n-6269-replace-vue-event-buses-with-custom

022424b

refactor: replace global Vue references in design-system

b17c3aa

alexgrozav changed the title ~~feat: replace Vue.extend with defineComponent in design system~~ feat: Replace Vue.extend with defineComponent in design system (no-changelog) Apr 6, 2023

n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Apr 6, 2023

refactor: update prop types

5555367

Base automatically changed from n8n-6269-replace-vue-event-buses-with-custom to master April 6, 2023 13:32

ivov reviewed Apr 6, 2023

View reviewed changes

alexgrozav added 4 commits April 10, 2023 16:16

chore: merge master

1e8bf8c

feat: improve types

9e58066

fix: further type improvements

08a5c81

fix: further types improvements

d8f4faf

ivov reviewed Apr 11, 2023

View reviewed changes

alexgrozav added 3 commits April 11, 2023 15:27

fix: further type improvements

3bf5b25

test: fix test snapshots

dbddd98

test: fix snapshot

b81b599

alexgrozav added 2 commits April 11, 2023 21:37

chore: fix linting issues

181b4c8

test: fix personalization modal snapshot

9431c63

ivov approved these changes Apr 12, 2023

View reviewed changes

alexgrozav merged commit 430a878 into master Apr 12, 2023

alexgrozav deleted the n8n-6268-replace-vueextend-with-definecomponent-design-system branch April 12, 2023 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Replace Vue.extend with defineComponent in design system (no-changelog) #5918

feat: Replace Vue.extend with defineComponent in design system (no-changelog) #5918

alexgrozav commented Apr 6, 2023

github-actions bot commented Apr 6, 2023 •

edited

Loading

codecov bot commented Apr 6, 2023 •

edited

Loading

ivov left a comment

ivov Apr 6, 2023

alexgrozav Apr 12, 2023

ivov Apr 6, 2023

ivov Apr 6, 2023

ivov Apr 6, 2023

ivov Apr 6, 2023

ivov Apr 6, 2023

ivov Apr 6, 2023

alexgrozav commented Apr 11, 2023

ivov left a comment

ivov Apr 11, 2023

ivov Apr 11, 2023

ivov Apr 11, 2023

ivov Apr 11, 2023

ivov Apr 11, 2023

ivov commented Apr 11, 2023

github-actions bot commented Apr 12, 2023

		@@ -1,4 +1,4 @@
		import Vue from 'vue';
		import { PluginObject } from 'vue';

	import { PluginObject } from 'vue';
	import type { PluginObject } from 'vue';

		@@ -0,0 +1,4 @@
		declare module 'markdown-it-task-lists' {
		import { PluginSimple } from 'markdown-it';

	import { PluginSimple } from 'markdown-it';
	import type { PluginSimple } from 'markdown-it';

feat: Replace Vue.extend with defineComponent in design system (no-changelog) #5918

feat: Replace Vue.extend with defineComponent in design system (no-changelog) #5918

Conversation

alexgrozav commented Apr 6, 2023

github-actions bot commented Apr 6, 2023 • edited Loading

codecov bot commented Apr 6, 2023 • edited Loading

Codecov Report

ivov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgrozav commented Apr 11, 2023

ivov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov commented Apr 11, 2023

github-actions bot commented Apr 12, 2023

github-actions bot commented Apr 6, 2023 •

edited

Loading

codecov bot commented Apr 6, 2023 •

edited

Loading