From cc90d0fb4e8412c5b52b2191e2b6f098596272ba Mon Sep 17 00:00:00 2001 From: Richard Fontein <32132657+rifont@users.noreply.github.com> Date: Wed, 30 Oct 2024 20:15:04 +0100 Subject: [PATCH 1/3] refactor(client): remove ora dependency and refactor logs --- packages/framework/package.json | 1 - packages/framework/src/client.test.ts | 37 +++++++++++ packages/framework/src/client.ts | 89 ++++++++++++--------------- 3 files changed, 75 insertions(+), 52 deletions(-) diff --git a/packages/framework/package.json b/packages/framework/package.json index 7cbb9cca9ae..37845e8fc46 100644 --- a/packages/framework/package.json +++ b/packages/framework/package.json @@ -241,7 +241,6 @@ "cross-fetch": "^4.0.0", "json-schema-to-ts": "^3.0.0", "liquidjs": "^10.13.1", - "ora": "^5.4.1", "sanitize-html": "^2.13.0" }, "nx": { diff --git a/packages/framework/src/client.test.ts b/packages/framework/src/client.test.ts index a79402e9ac6..26554e3811e 100644 --- a/packages/framework/src/client.test.ts +++ b/packages/framework/src/client.test.ts @@ -1287,6 +1287,43 @@ describe('Novu Client', () => { expect(mockFn).toHaveBeenCalledTimes(0); }); + it.only('should NOT log anything after executing the provided stepId', async () => { + const mockFn = vi.fn(); + const spyConsoleLog = vi.spyOn(console, 'log'); + const newWorkflow = workflow('test-workflow', async ({ step }) => { + await step.email('active-step-id', async () => ({ body: 'Test Body', subject: 'Subject' })); + await step.email('inactive-step-id', async () => { + mockFn(); + + return { body: 'Test Body', subject: 'Subject' }; + }); + }); + + client.addWorkflows([newWorkflow]); + + const event: Event = { + action: PostActionEnum.EXECUTE, + workflowId: 'test-workflow', + stepId: 'active-step-id', + subscriber: {}, + state: [], + payload: {}, + controls: {}, + }; + + await client.executeWorkflow(event); + + // Wait for the conclusion promise to resolve. + await new Promise((resolve) => { + setTimeout(resolve); + }); + /* + * Not the most robust test, but ensures that the last log call contains the duration, + * which is the last expected log call. + */ + expect(spyConsoleLog.mock.lastCall).toEqual([expect.stringContaining('duration:')]); + }); + it('should evaluate code in steps after a skipped step', async () => { const mockFn = vi.fn(); const newWorkflow = workflow('test-workflow', async ({ step }) => { diff --git a/packages/framework/src/client.ts b/packages/framework/src/client.ts index f2da90a3338..4a28f534d66 100644 --- a/packages/framework/src/client.ts +++ b/packages/framework/src/client.ts @@ -1,5 +1,4 @@ import { Liquid } from 'liquidjs'; -import ora from 'ora'; import { ChannelStepEnum, PostActionEnum } from './constants'; import { @@ -257,9 +256,18 @@ export class Client { private executeStepFactory, T_Result extends Record>( event: Event, - setResult: (result: Pick) => void + setResult: (result: Pick) => void, + hasResult: () => boolean ): ActionStep { return async (stepId, stepResolve, options) => { + if (hasResult()) { + /* + * Exit the execution early if the result has already been set. + * This is to ensure that we don't evaluate code in steps after the provided stepId. + */ + return; + } + const step = this.getStep(event.workflowId, stepId); const controls = await this.createStepControls(step, event); const isPreview = event.action === PostActionEnum.PREVIEW; @@ -364,6 +372,7 @@ export class Client { }; let concludeExecution: (value?: unknown) => void; + let hasConcludedExecution = false; const concludeExecutionPromise = new Promise((resolve) => { concludeExecution = resolve; }); @@ -375,15 +384,20 @@ export class Client { * `workflow.execute` method. By resolving the `concludeExecutionPromise` when setting the result, * we can ensure that the `workflow.execute` method is not evaluated after the `stepId` is reached. * - * This function should only be called once per workflow execution. - * * @param stepResult The result of the workflow execution. */ const setResult = (stepResult: Omit): void => { + if (hasConcludedExecution) { + throw new Error('setResult can only be called once per workflow execution'); + } concludeExecution(); + hasConcludedExecution = true; + result = stepResult; }; + const hasResult = (): boolean => hasConcludedExecution; + let executionError: Error | undefined; try { if ( @@ -408,14 +422,14 @@ export class Client { controls: {}, subscriber: event.subscriber, step: { - email: this.executeStepFactory(validatedEvent, setResult), - sms: this.executeStepFactory(validatedEvent, setResult), - inApp: this.executeStepFactory(validatedEvent, setResult), - digest: this.executeStepFactory(validatedEvent, setResult), - delay: this.executeStepFactory(validatedEvent, setResult), - push: this.executeStepFactory(validatedEvent, setResult), - chat: this.executeStepFactory(validatedEvent, setResult), - custom: this.executeStepFactory(validatedEvent, setResult), + email: this.executeStepFactory(validatedEvent, setResult, hasResult), + sms: this.executeStepFactory(validatedEvent, setResult, hasResult), + inApp: this.executeStepFactory(validatedEvent, setResult, hasResult), + digest: this.executeStepFactory(validatedEvent, setResult, hasResult), + delay: this.executeStepFactory(validatedEvent, setResult, hasResult), + push: this.executeStepFactory(validatedEvent, setResult, hasResult), + chat: this.executeStepFactory(validatedEvent, setResult, hasResult), + custom: this.executeStepFactory(validatedEvent, setResult, hasResult), }, }), ]); @@ -541,7 +555,6 @@ export class Client { provider: DiscoverProviderOutput, outputs: Record ): Promise>> { - const spinner = ora({ indent: 2 }).start(`Executing provider: \`${provider.type}\``); try { if (event.stepId === step.stepId) { const controls = await this.createStepControls(step, event); @@ -558,7 +571,7 @@ export class Client { step.stepId, provider.type ); - spinner.succeed(`Executed provider: \`${provider.type}\``); + console.log(` ${EMOJI.SUCCESS} Executed provider: \`${provider.type}\``); return { ...validatedOutput, @@ -566,18 +579,13 @@ export class Client { }; } else { // No-op. We don't execute providers for hydrated steps - spinner.stopAndPersist({ - symbol: EMOJI.HYDRATED, - text: `Hydrated provider: \`${provider.type}\``, - }); + console.log(` ${EMOJI.HYDRATED} Hydrated provider: \`${provider.type}\``); return {}; } } catch (error) { - spinner.stopAndPersist({ - symbol: EMOJI.ERROR, - text: `Failed to execute provider: \`${provider.type}\``, - }); + console.log(` ${EMOJI.ERROR} Failed to execute provider: \`${provider.type}\``); + throw new ProviderExecutionFailedError(provider.type, event.action, error); } } @@ -587,7 +595,6 @@ export class Client { step: DiscoverStepOutput ): Promise> { if (event.stepId === step.stepId) { - const spinner = ora({ indent: 1 }).start(`Executing stepId: \`${step.stepId}\``); try { const templateControls = await this.createStepControls(step, event); const controls = await this.compileControls(templateControls, event); @@ -603,18 +610,14 @@ export class Client { const providers = await this.executeProviders(event, step, validatedOutput); - spinner.succeed(`Executed stepId: \`${step.stepId}\``); + console.log(` ${EMOJI.SUCCESS} Executed stepId: \`${step.stepId}\``); return { outputs: validatedOutput, providers, }; } catch (error) { - spinner.stopAndPersist({ - prefixText: '', - symbol: EMOJI.ERROR, - text: `Failed to execute stepId: \`${step.stepId}\``, - }); + console.log(` ${EMOJI.ERROR} Failed to execute stepId: \`${step.stepId}\``); if (isFrameworkError(error)) { throw error; } else { @@ -622,7 +625,6 @@ export class Client { } } } else { - const spinner = ora({ indent: 1 }).start(`Hydrating stepId: \`${step.stepId}\``); try { const result = event.state.find((state) => state.stepId === step.stepId); @@ -635,10 +637,7 @@ export class Client { event.workflowId, step.stepId ); - spinner.stopAndPersist({ - symbol: EMOJI.HYDRATED, - text: `Hydrated stepId: \`${step.stepId}\``, - }); + console.log(` ${EMOJI.HYDRATED} Hydrated stepId: \`${step.stepId}\``); return { outputs: validatedOutput, @@ -648,10 +647,8 @@ export class Client { throw new ExecutionStateCorruptError(event.workflowId, step.stepId); } } catch (error) { - spinner.stopAndPersist({ - symbol: EMOJI.ERROR, - text: `Failed to hydrate stepId: \`${step.stepId}\``, - }); + console.log(` ${EMOJI.ERROR} Failed to hydrate stepId: \`${step.stepId}\``); + throw error; } } @@ -696,7 +693,6 @@ export class Client { event: Event, step: DiscoverStepOutput ): Promise> { - const spinner = ora({ indent: 1 }).start(`Previewing stepId: \`${step.stepId}\``); try { if (event.stepId === step.stepId) { const templateControls = await this.createStepControls(step, event); @@ -712,10 +708,7 @@ export class Client { step.stepId ); - spinner.stopAndPersist({ - symbol: EMOJI.MOCK, - text: `Mocked stepId: \`${step.stepId}\``, - }); + console.log(` ${EMOJI.MOCK} Mocked stepId: \`${step.stepId}\``); return { outputs: validatedOutput, @@ -724,10 +717,7 @@ export class Client { } else { const mockResult = this.mock(step.results.schema); - spinner.stopAndPersist({ - symbol: EMOJI.MOCK, - text: `Mocked stepId: \`${step.stepId}\``, - }); + console.log(` ${EMOJI.MOCK} Mocked stepId: \`${step.stepId}\``); return { outputs: mockResult, @@ -735,10 +725,7 @@ export class Client { }; } } catch (error) { - spinner.stopAndPersist({ - symbol: EMOJI.ERROR, - text: `Failed to preview stepId: \`${step.stepId}\``, - }); + console.log(` ${EMOJI.ERROR} Failed to preview stepId: \`${step.stepId}\``); if (isFrameworkError(error)) { throw error; From 8651ab640d8c3213d104cf25c0b3a8294fdc015c Mon Sep 17 00:00:00 2001 From: Richard Fontein <32132657+rifont@users.noreply.github.com> Date: Wed, 30 Oct 2024 20:15:17 +0100 Subject: [PATCH 2/3] build: update dependencies in pnpm-lock.yaml --- pnpm-lock.yaml | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f1075e2d508..b0eaef83159 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -3396,9 +3396,6 @@ importers: liquidjs: specifier: ^10.13.1 version: 10.13.1 - ora: - specifier: ^5.4.1 - version: 5.4.1 sanitize-html: specifier: ^2.13.0 version: 2.13.0 @@ -26654,8 +26651,8 @@ packages: engines: {node: '>= 14.0.0'} hasBin: true - mocha@10.7.3: - resolution: {integrity: sha512-uQWxAu44wwiACGqjbPYmjo7Lg8sFrS3dQe7PP2FQI+woptP4vZXSMcfMyFL/e1yFEeEpV4RtyTpZROOKmxis+A==} + mocha@10.8.2: + resolution: {integrity: sha512-VZlYo/WE8t1tstuRmqgeyBgCbJc/lEdopaa+axcKzTBJ+UIdlAB9XnmvTCAH4pwR4ElNInaedhEBmZD8iCSVEg==} engines: {node: '>= 14.0.0'} hasBin: true @@ -34989,8 +34986,8 @@ snapshots: dependencies: '@aws-crypto/sha256-browser': 3.0.0 '@aws-crypto/sha256-js': 3.0.0 - '@aws-sdk/client-sso-oidc': 3.575.0 - '@aws-sdk/client-sts': 3.575.0(@aws-sdk/client-sso-oidc@3.575.0) + '@aws-sdk/client-sso-oidc': 3.575.0(@aws-sdk/client-sts@3.575.0) + '@aws-sdk/client-sts': 3.575.0 '@aws-sdk/core': 3.575.0 '@aws-sdk/credential-provider-node': 3.575.0(@aws-sdk/client-sso-oidc@3.575.0)(@aws-sdk/client-sts@3.575.0) '@aws-sdk/middleware-host-header': 3.575.0 @@ -35191,8 +35188,8 @@ snapshots: '@aws-crypto/sha1-browser': 3.0.0 '@aws-crypto/sha256-browser': 3.0.0 '@aws-crypto/sha256-js': 3.0.0 - '@aws-sdk/client-sso-oidc': 3.575.0 - '@aws-sdk/client-sts': 3.575.0(@aws-sdk/client-sso-oidc@3.575.0) + '@aws-sdk/client-sso-oidc': 3.575.0(@aws-sdk/client-sts@3.575.0) + '@aws-sdk/client-sts': 3.575.0 '@aws-sdk/core': 3.575.0 '@aws-sdk/credential-provider-node': 3.575.0(@aws-sdk/client-sso-oidc@3.575.0)(@aws-sdk/client-sts@3.575.0) '@aws-sdk/middleware-bucket-endpoint': 3.575.0 @@ -35418,11 +35415,11 @@ snapshots: - aws-crt optional: true - '@aws-sdk/client-sso-oidc@3.575.0': + '@aws-sdk/client-sso-oidc@3.575.0(@aws-sdk/client-sts@3.575.0)': dependencies: '@aws-crypto/sha256-browser': 3.0.0 '@aws-crypto/sha256-js': 3.0.0 - '@aws-sdk/client-sts': 3.575.0(@aws-sdk/client-sso-oidc@3.575.0) + '@aws-sdk/client-sts': 3.575.0 '@aws-sdk/core': 3.575.0 '@aws-sdk/credential-provider-node': 3.575.0(@aws-sdk/client-sso-oidc@3.575.0)(@aws-sdk/client-sts@3.575.0) '@aws-sdk/middleware-host-header': 3.575.0 @@ -35461,6 +35458,7 @@ snapshots: '@smithy/util-utf8': 3.0.0 tslib: 2.7.0 transitivePeerDependencies: + - '@aws-sdk/client-sts' - aws-crt '@aws-sdk/client-sso-oidc@3.637.0(@aws-sdk/client-sts@3.637.0)': @@ -35845,11 +35843,11 @@ snapshots: - aws-crt optional: true - '@aws-sdk/client-sts@3.575.0(@aws-sdk/client-sso-oidc@3.575.0)': + '@aws-sdk/client-sts@3.575.0': dependencies: '@aws-crypto/sha256-browser': 3.0.0 '@aws-crypto/sha256-js': 3.0.0 - '@aws-sdk/client-sso-oidc': 3.575.0 + '@aws-sdk/client-sso-oidc': 3.575.0(@aws-sdk/client-sts@3.575.0) '@aws-sdk/core': 3.575.0 '@aws-sdk/credential-provider-node': 3.575.0(@aws-sdk/client-sso-oidc@3.575.0)(@aws-sdk/client-sts@3.575.0) '@aws-sdk/middleware-host-header': 3.575.0 @@ -35888,7 +35886,6 @@ snapshots: '@smithy/util-utf8': 3.0.0 tslib: 2.7.0 transitivePeerDependencies: - - '@aws-sdk/client-sso-oidc' - aws-crt '@aws-sdk/client-sts@3.637.0': @@ -36118,7 +36115,7 @@ snapshots: '@aws-sdk/credential-provider-ini@3.575.0(@aws-sdk/client-sso-oidc@3.575.0)(@aws-sdk/client-sts@3.575.0)': dependencies: - '@aws-sdk/client-sts': 3.575.0(@aws-sdk/client-sso-oidc@3.575.0) + '@aws-sdk/client-sts': 3.575.0 '@aws-sdk/credential-provider-env': 3.575.0 '@aws-sdk/credential-provider-process': 3.575.0 '@aws-sdk/credential-provider-sso': 3.575.0(@aws-sdk/client-sso-oidc@3.575.0) @@ -36429,7 +36426,7 @@ snapshots: '@aws-sdk/credential-provider-web-identity@3.575.0(@aws-sdk/client-sts@3.575.0)': dependencies: - '@aws-sdk/client-sts': 3.575.0(@aws-sdk/client-sso-oidc@3.575.0) + '@aws-sdk/client-sts': 3.575.0 '@aws-sdk/types': 3.575.0 '@smithy/property-provider': 3.1.3 '@smithy/types': 3.3.0 @@ -36950,7 +36947,7 @@ snapshots: '@aws-sdk/token-providers@3.575.0(@aws-sdk/client-sso-oidc@3.575.0)': dependencies: - '@aws-sdk/client-sso-oidc': 3.575.0 + '@aws-sdk/client-sso-oidc': 3.575.0(@aws-sdk/client-sts@3.575.0) '@aws-sdk/types': 3.575.0 '@smithy/property-provider': 3.1.3 '@smithy/shared-ini-file-loader': 3.1.4 @@ -36959,7 +36956,7 @@ snapshots: '@aws-sdk/token-providers@3.614.0(@aws-sdk/client-sso-oidc@3.575.0)': dependencies: - '@aws-sdk/client-sso-oidc': 3.575.0 + '@aws-sdk/client-sso-oidc': 3.575.0(@aws-sdk/client-sts@3.575.0) '@aws-sdk/types': 3.609.0 '@smithy/property-provider': 3.1.3 '@smithy/shared-ini-file-loader': 3.1.4 @@ -62105,7 +62102,7 @@ snapshots: cypress-intellij-reporter@0.0.7: dependencies: - mocha: 10.7.3 + mocha: 10.8.2 cypress-network-idle@1.14.2: {} @@ -71266,7 +71263,7 @@ snapshots: yargs-parser: 20.2.4 yargs-unparser: 2.0.0 - mocha@10.7.3: + mocha@10.8.2: dependencies: ansi-colors: 4.1.3 browser-stdout: 1.3.1 From 38704e4b899327b52388002b41566375106d2138 Mon Sep 17 00:00:00 2001 From: Richard Fontein <32132657+rifont@users.noreply.github.com> Date: Wed, 30 Oct 2024 20:26:46 +0100 Subject: [PATCH 3/3] test: remove .only from test case --- packages/framework/src/client.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/framework/src/client.test.ts b/packages/framework/src/client.test.ts index 26554e3811e..2dd70587c12 100644 --- a/packages/framework/src/client.test.ts +++ b/packages/framework/src/client.test.ts @@ -1287,7 +1287,7 @@ describe('Novu Client', () => { expect(mockFn).toHaveBeenCalledTimes(0); }); - it.only('should NOT log anything after executing the provided stepId', async () => { + it('should NOT log anything after executing the provided stepId', async () => { const mockFn = vi.fn(); const spyConsoleLog = vi.spyOn(console, 'log'); const newWorkflow = workflow('test-workflow', async ({ step }) => {