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): Fix remaining FE type check errors (no-changelog) #9607

Merged
merged 18 commits into from
Jun 10, 2024

Conversation

RicardoE105
Copy link
Contributor

@RicardoE105 RicardoE105 commented Jun 3, 2024

Summary

  • Reduce the number of type errors in the FE from 118 to 0.
  • Enable type checking as part of build.

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)

@RicardoE105 RicardoE105 changed the title fix more type errors in the FE Fix(editor-ui) More type errors in the FE (no-changelog) Jun 3, 2024
@RicardoE105 RicardoE105 changed the title Fix(editor-ui) More type errors in the FE (no-changelog) fix(editor): More type errors in the FE (no-changelog) Jun 3, 2024
@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Jun 3, 2024
@RicardoE105 RicardoE105 requested a review from mutdmour June 4, 2024 01:15
@@ -20,7 +21,7 @@ export function compileFakeDoorFeatures(): IFakeDoor[] {
if (loggingFeature) {
loggingFeature.actionBoxTitle += '.cloud';
loggingFeature.linkURL += '&edition=cloud';
loggingFeature.infoText = '';
loggingFeature.infoText = '' as BaseTextKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a valid basetext key? maybe we need to redefine the types here somehow?

@alexgrozav alexgrozav changed the title fix(editor): More type errors in the FE (no-changelog) refactor(editor): Fix remaining FE type check errors (no-changelog) Jun 5, 2024
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 thank you for working on this 🙏🏽

@@ -240,7 +240,7 @@ const validationError = computed<string | null>(() => {

if (error) {
if ('messageKey' in error) {
return t(error.messageKey, error.options as object);
return t(error.messageKey, error.options as string[]);
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be better defined in the original type?

Copy link
Member

Choose a reason for hiding this comment

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

Updated.

@@ -499,7 +499,7 @@ export default defineComponent({

this.credentialData = {
...this.credentialData,
scopes,
scopes: scopes as unknown as CredentialInformation,
Copy link
Contributor

Choose a reason for hiding this comment

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

could we define those better in the original type?

Copy link
Member

Choose a reason for hiding this comment

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

Updated.

return sharee.id === this.usersStore.currentUser?.id;
});
},
projects(): ProjectListItem[] {
return this.projectsStore.personalProjects.filter(
(project) =>
project.id !== this.credential?.homeProject?.id &&
project.id !== this.credentialData?.homeProject?.id,
project.id !== (this.credentialData?.homeProject as IDataObject)?.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

could we define this better in the original object? I know cred types are messy, but we should avoid bandaid solutions here.

Copy link
Member

Choose a reason for hiding this comment

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

Updated.

@@ -77,7 +77,8 @@ const plugins = [
}),
vue(),
];
if (process.env.ENABLE_TYPE_CHECKING === 'true') {

if (!process.env.VITEST) {
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

@@ -50,7 +50,8 @@ app.mount('#app');
if (!import.meta.env.PROD) {
// Make sure that we get all error messages properly displayed
// as long as we are not in production mode
window.onerror = (message, source, lineno, colno, error) => {
window.onerror = (message, _source, _lineno, _colno, error) => {
// eslint-disable-next-line @typescript-eslint/no-base-to-string
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be handled differently?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. What do you have in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

will trust your judgement here

@@ -517,7 +517,7 @@ export class N8nConnector extends AbstractConnector {
}

resetTargetEndpoint() {
this.overrideTargetEndpoint = null;
this.overrideTargetEndpoint = null as unknown as Endpoint;
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like we have to fix the types here.

Copy link
Member

Choose a reason for hiding this comment

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

Not much I can do. We're extending AbstractEndpoint and just using different types. I'm afraid of the side effects of removing all those null assignments. Should we try to tackle this separately?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure maybe we can add a todo or open a story?

Copy link
Member

Choose a reason for hiding this comment

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

packages/editor-ui/src/utils/apiUtils.ts Outdated Show resolved Hide resolved
@@ -116,21 +116,21 @@ export interface IUser {
lastName: string;
}

export type ProjectSharingData = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks 😍

mutdmour
mutdmour previously approved these changes Jun 10, 2024
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. Thanks for addressing my feedback

@@ -73,3 +73,9 @@ export function isTriggerPanelObject(
): triggerPanel is TriggerPanelDefinition {
return triggerPanel !== undefined && typeof triggerPanel === 'object' && triggerPanel !== null;
}

export function isFullExecutionResponse(
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

mutdmour
mutdmour previously approved these changes Jun 10, 2024
Copy link

cypress bot commented Jun 10, 2024

2 flaky tests on run #5388 ↗︎

0 364 0 0 Flakiness 2

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 RicardoE105 🗃️ e2e/*
Project: n8n Commit: 2f34777f0a
Status: Passed Duration: 04:58 💡
Started: Jun 10, 2024 12:59 PM Ended: Jun 10, 2024 1:04 PM
Flakiness  5-ndv.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > Stop listening for trigger event from NDV Screenshots Video
Flakiness  20-workflow-executions.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Current Workflow Executions > should auto load more items if there is space and auto scroll Test Replay Screenshots Video

Review all test suite changes for PR #9607 ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

Copy link
Contributor

✅ All Cypress E2E specs passed

@alexgrozav alexgrozav merged commit 22bdb05 into master Jun 10, 2024
44 checks passed
@alexgrozav alexgrozav deleted the fe-type-errors branch June 10, 2024 13:23
MiloradFilipovic added a commit that referenced this pull request Jun 10, 2024
* master:
  feat(editor): Harmonize rendering of new-lines in RunData (#9614)
  refactor(editor): Fix remaining FE type check errors (no-changelog) (#9607)
  fix(editor): Remove template creds setup from workflow when copied (no-changelog) (#9673)
  refactor(editor): Stop expecting `null` execution status (no-changelog) (#9672)
MiloradFilipovic added a commit that referenced this pull request Jun 10, 2024
* master:
  feat(editor): Harmonize rendering of new-lines in RunData (#9614)
  refactor(editor): Fix remaining FE type check errors (no-changelog) (#9607)
  fix(editor): Remove template creds setup from workflow when copied (no-changelog) (#9673)
  refactor(editor): Stop expecting `null` execution status (no-changelog) (#9672)
  fix: Fix typo with submitted (no-changelog) (#9662)
  refactor(core): Revamp crash recovery mechanism for main mode (#9613)
  fix(editor): Improve dragndrop of input pills with spaces (#9656)
  fix(editor): Indent on tabs in expression fields (#9659)
  fix(editor): Color node connections correctly in execution preview for nodes that have pinned data (#9669)
  fix(core): Fix optional chaining in continue on fail check (#9667)
  feat(OpenAI Node): Allow to select Image analyze model & improve types (#9660)
@janober
Copy link
Member

janober commented Jun 12, 2024

Got released with [email protected]

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 Released ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants