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

refactor(editor): Migrate part of the vuex store to pinia #4484

Merged
merged 44 commits into from
Nov 4, 2022

Conversation

MiloradFilipovic
Copy link
Contributor

Closes N8N-5193

# Conflicts:
#	packages/editor-ui/src/components/ActivationModal.vue
#	packages/editor-ui/src/components/ImportParameter.vue
#	packages/editor-ui/src/components/MainHeader/MainHeader.vue
#	packages/editor-ui/src/components/MainHeader/WorkflowDetails.vue
#	packages/editor-ui/src/components/MainSidebar.vue
#	packages/editor-ui/src/components/mixins/mouseSelect.ts
#	packages/editor-ui/src/components/mixins/nodeBase.ts
#	packages/editor-ui/src/components/mixins/nodeHelpers.ts
#	packages/editor-ui/src/modules/settings.ts
#	packages/editor-ui/src/plugins/i18n/index.ts
#	packages/editor-ui/src/router.ts
#	packages/editor-ui/src/views/NodeView.vue
…part1

# Conflicts:
#	packages/editor-ui/src/components/ExecutionsView/ExecutionsView.vue
#	packages/editor-ui/src/views/NodeView.vue
…part1

# Conflicts:
#	packages/editor-ui/src/components/WorkflowSettings.vue
#	packages/editor-ui/src/components/mixins/workflowHelpers.ts
#	packages/editor-ui/src/store.ts
#	packages/editor-ui/src/views/NodeView.vue
@linear
Copy link

linear bot commented Oct 31, 2022

N8N-5193

N8N-5190

@MiloradFilipovic MiloradFilipovic changed the title refactor(editor): pinia migration phase 1 (no-changelog) refactor(editor): migrate part of the vuex store to pinia (no-changelog) Oct 31, 2022
@MiloradFilipovic MiloradFilipovic changed the title refactor(editor): migrate part of the vuex store to pinia (no-changelog) refactor(editor): migrate part of the vuex store to pinia Nov 2, 2022
Copy link
Contributor

@mutdmour mutdmour left a comment

Choose a reason for hiding this comment

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

GREAT JOB

packages/editor-ui/src/Interface.ts Outdated Show resolved Hide resolved
packages/editor-ui/src/components/NDVDraggablePanels.vue Outdated Show resolved Hide resolved
packages/editor-ui/src/components/mixins/pushConnection.ts Outdated Show resolved Hide resolved
packages/editor-ui/src/views/NodeView.vue Show resolved Hide resolved
packages/editor-ui/src/views/NodeView.vue Outdated Show resolved Hide resolved
packages/editor-ui/src/views/NodeView.vue Show resolved Hide resolved
packages/editor-ui/src/views/NodeView.vue Outdated Show resolved Hide resolved
packages/editor-ui/src/views/SettingsApiView.vue Outdated Show resolved Hide resolved
position,
} as IDataObject,
} as INodeUpdatePropertiesInformation;
position: { position },
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm if this works.. then can you please for other places we change node positions? like in pushDownstreamNodes in NodeView.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to work, but the code looks really weird. Take a look.

Comment on lines 113 to 114
const token = this.$route.query.token.toString();
const userId = this.$route.query.userId.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

should not we check here still?

Comment on lines 116 to 117
const inviterId = this.$route.query.inviterId.toString();
const inviteeId = this.$route.query.inviteeId.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

should not this be addressed still?

Comment on lines 206 to 210
const updateInformation = {
key: this.node.id,
name: this.node.name,
value: nodeParameters,
};
} as IUpdateInformation;
Copy link
Contributor

Choose a reason for hiding this comment

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

still needs to be addressed

mutdmour
mutdmour previously approved these changes Nov 4, 2022
Copy link
Contributor

@mutdmour mutdmour left a comment

Choose a reason for hiding this comment

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

Awesome. just a couple of minor comments

const inviterId = this.$route.query.inviterId.toString();
const inviteeId = this.$route.query.inviteeId.toString();
const inviterId = (!this.$route.query.inviterId || typeof this.$route.query.inviterId !== 'string') ? null : this.$route.query.inviterId;
const inviteeId = (!this.$route.query.inviteeId || typeof this.$route.query.inviteeId !== 'string') ? null : this.$route.query.inviteeId;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an edge case.. maybe better to set inviterId and inviteeId as data and so we don't need to check types here.. anyways we should not continue to signup with these values as null

@MiloradFilipovic MiloradFilipovic changed the title refactor(editor): migrate part of the vuex store to pinia refactor(editor): Migrate part of the vuex store to pinia Nov 4, 2022
@MiloradFilipovic MiloradFilipovic merged commit 40e413d into master Nov 4, 2022
@MiloradFilipovic MiloradFilipovic deleted the N8N-5190-pinia-migration-part1 branch November 4, 2022 13:04
@n8n-assistant n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Nov 4, 2022
MiloradFilipovic added a commit that referenced this pull request Nov 4, 2022
* master:
  refactor(editor): Migrate part of the vuex store to pinia (#4484)
  fix(editor): fix themes the monaco editor (no-changelog) (#4521)
  refactor(editor): restrict mapping discoverability tooltip showing only on string input (#4496)
  📚 Remove entries from Changelog
  📚 Update CHANGELOG.md and main package.json to 0.201.0
  🔖 Release [email protected]
  ⬆️ Set [email protected], [email protected], [email protected] and [email protected] on n8n
  🔖 Release [email protected]
  ⬆️ Set [email protected] and [email protected] on n8n-editor-ui
  🔖 Release [email protected]
  🔖 Release [email protected]
  ⬆️ Set [email protected] and [email protected] on n8n-nodes-base
  🔖 Release [email protected]
  ⬆️ Set [email protected] and [email protected] on n8n-node-dev
  🔖 Release [email protected]
  ⬆️ Set [email protected] on n8n-core
  🔖 Release [email protected]
  fix(core): make `deepCopy` backward compatible (#4505)
  fix(editor): workflow settings link font size change (no-changelog) (#4511)
  perf(editor): improve array intersection utility function (#4503)

# Conflicts:
#	packages/editor-ui/src/App.vue
#	packages/editor-ui/src/components/ActivationModal.vue
#	packages/editor-ui/src/components/NDVDraggablePanels.vue
#	packages/editor-ui/src/components/NodeSettings.vue
#	packages/editor-ui/src/components/Sticky.vue
#	packages/editor-ui/src/components/mixins/workflowHelpers.ts
#	packages/editor-ui/src/constants.ts
#	packages/editor-ui/src/plugins/i18n/index.ts
#	packages/editor-ui/src/views/ChangePasswordView.vue
#	packages/editor-ui/src/views/NodeView.vue
#	packages/editor-ui/src/views/SignupView.vue
@janober
Copy link
Member

janober commented Nov 10, 2022

Got released with [email protected]

@janober janober removed the Upcoming Release Will be part of the upcoming release label Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants