refactor(editor): Migrate nodeBase mixin to composable (no-changelog) #9742
Conversation
n8n-assistant
bot
added
n8n team
| type: Boolean, | ||
| }, | ||
| workflow: { | ||
| type: Object as () => Workflow, |
There was a problem hiding this comment.
Is this correct? should it be PropType<Workflow>?
There was a problem hiding this comment.
Oops, copy pasted it. Good catch! Fixed.
| setForceActions, | ||
| }; | ||
| }, | ||
| data() { | ||
| return { | ||
| isResizing: false, | ||
| isTouchActive: false, | ||
| inputs: [] as Array<ConnectionTypes | INodeInputConfiguration>, | ||
| outputs: [] as Array<ConnectionTypes | INodeOutputConfiguration>, |
There was a problem hiding this comment.
Isn't this overridden by ...nodeBase in setup? (contains inputs/outputs refs)
There was a problem hiding this comment.
You're right! Removed.
| return { | ||
| contextMenu, | ||
| externalHooks, | ||
| nodeHelpers, | ||
| pinnedData, | ||
| deviceSupport, | ||
| ...nodeBase, |
There was a problem hiding this comment.
nit: Would prefer without the spread, but I understand if it's to limit the amount of refactoring in this PR
There was a problem hiding this comment.
Good point. Fixed.
There was a problem hiding this comment.
Had to undo the change. Refs and typechecks don't play well with options API. 😰
| name: { | ||
| type: String, | ||
| required: true, | ||
| }, | ||
| instance: { | ||
| type: Object as PropType<BrowserJsPlumbInstance>, | ||
| required: true, | ||
| }, | ||
| isReadOnly: { | ||
| type: Boolean, | ||
| }, | ||
| isActive: { | ||
| type: Boolean, | ||
| }, | ||
| hideActions: { | ||
| type: Boolean, | ||
| }, | ||
| disableSelecting: { | ||
| type: Boolean, | ||
| }, | ||
| showCustomTooltip: { | ||
| type: Boolean, | ||
| }, | ||
| workflow: { | ||
| type: Object as () => Workflow, | ||
| required: true, | ||
| }, |
There was a problem hiding this comment.
is there a plan for how we want to tackle this props duplication once we have migrated away from defineComponent ?
There was a problem hiding this comment.
Props interfaces should work I think.
| mounted() { | ||
| // Initialize the node | ||
| if (this.data !== null) { | ||
| try { | ||
| this.addNode(this.data); | ||
| } catch (error) { | ||
| // This breaks when new nodes are loaded into store but workflow tab is not currently active | ||
| // Shouldn't affect anything | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
same question here about code duplication.
There was a problem hiding this comment.
We'll probably need a composable after we rewrite.
| import { isValidNodeConnectionType } from '@/utils/typeGuards'; | ||
| import { useI18n } from '@/composables/useI18n'; | ||
|
|
||
| export function useNodeBase({ |
There was a problem hiding this comment.
Would you like to add some tests for this in the same PR? or should we try to do that in a separate PR ?
There was a problem hiding this comment.
Added some tests for main functions now
|
✅ No visual regressions found. |
|
✅ All Cypress E2E specs passed |
|
✅ All Cypress E2E specs passed |
|
Got released with |
1 similar comment
|
Got released with |
Summary
Migrates
nodeBasemixin touseNodeBasecomposable.Related Linear tickets, Github issues, and Community forum posts
N/A
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)