refactor(core): Refactor nodes loading (no-changelog) #7283

netroy · 2023-09-27T19:52:08Z

This also fixes PAY-605

github-actions · 2023-09-27T19:52:22Z

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.

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

ivov

Tested manually, still working, only minor comments.

ivov · 2023-09-29T10:43:59Z

packages/cli/src/Server.ts

@@ -571,9 +570,11 @@ export class Server extends AbstractServer {
 		}

 		if (config.getEnv('nodes.communityPackages.enabled')) {
-			controllers.push(
-				new NodesController(config, this.loadNodesAndCredentials, this.push, internalHooks),
+			// eslint-disable-next-line @typescript-eslint/naming-convention


Now or later, maybe it's time to relax or remove this rule. Every dynamically loaded class will trigger it.

packages/cli/src/controllers/communityPackages.controller.ts

packages/cli/src/LoadNodesAndCredentials.ts

This also fixes PAY-605

codecov · 2023-09-29T17:53:07Z

Codecov Report

Attention: 161 lines in your changes are missing coverage. Please review.

Comparison is base (789e1e7) 33.46% compared to head (5a59e1f) 33.41%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7283      +/-   ##
==========================================
- Coverage   33.46%   33.41%   -0.05%     
==========================================
  Files        3389     3389              
  Lines      206599   206610      +11     
  Branches    22288    22284       -4     
==========================================
- Hits        69143    69049      -94     
- Misses     136337   136444     +107     
+ Partials     1119     1117       -2

Files	Coverage Δ
packages/cli/src/CredentialsOverwrites.ts	`28.84% <100.00%> (-0.25%)`	⬇️
packages/cli/src/WorkflowRunnerProcess.ts	`0.00% <ø> (ø)`
packages/cli/src/commands/BaseCommand.ts	`31.91% <100.00%> (-2.78%)`	⬇️
packages/cli/src/config/schema.ts	`81.81% <ø> (ø)`
packages/cli/src/controllers/index.ts	`100.00% <ø> (ø)`
packages/workflow/src/Interfaces.ts	`77.77% <ø> (ø)`
packages/cli/src/CredentialsHelper.ts	`23.45% <75.00%> (-0.34%)`	⬇️
packages/cli/src/NodeTypes.ts	`31.37% <75.00%> (-5.47%)`	⬇️
packages/cli/src/audit/risks/nodes.risk.ts	`82.05% <80.00%> (-1.74%)`	⬇️
packages/cli/src/commands/worker.ts	`26.72% <0.00%> (ø)`
... and 8 more

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

netroy · 2023-10-02T11:39:29Z

packages/cli/src/LoadNodesAndCredentials.ts

@@ -50,20 +40,20 @@ export class LoadNodesAndCredentials implements INodesAndCredentials {

 	includeNodes = config.getEnv('nodes.include');

-	credentialTypes: ICredentialTypes;


this dependency tree was too messy. by moving code out into CommunityPackagesService and FrontendService, this class is a lot more leaner, and there are fewer cyclic dependencies now.
Also, the code that got moved out isn't loaded in webhook and worker services anymore.

netroy · 2023-10-02T11:40:07Z

packages/cli/src/services/frontend.service.ts

+import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials';
+
+@Service()
+export class FrontendService {


will move all fronted-settings out of Server.ts and into this class in a separate PR.

I can take this over as well if you like.

packages/cli/src/NodeTypes.ts

packages/cli/src/Server.ts

packages/cli/src/controllers/communityPackages.controller.ts

packages/cli/src/services/communityPackages.service.ts

packages/cli/src/NodeTypes.ts

packages/cli/src/services/frontend.service.ts

ivov · 2023-10-05T08:35:21Z

packages/cli/test/integration/commands/import.cmd.test.ts


-LoggerProxy.init(getLogger());
+LoggerProxy.init(mock<ILogger>());


I love this, we should be mocking the logger more often.

I have a branch to convert Logger into a service, and mock that everywhere. no more LoggerProxy in the cli package.

ivov · 2023-10-05T08:39:30Z

packages/cli/src/CredentialsOverwrites.ts


-class CredentialsOverwritesClass {
+@Service()
+export class CredentialsOverwrites {


For understanding: What's the difference between the overwrites that happen in CredentialsOverwrites vs. the overwrites that happen in the FrontendService? Why do we need both and for them to be separate?

The overwrites in FrontendService are only used in the frontend, and that code should not run on worker or webhooks. That said, we could still move that code into a method on CredentialsOverwrites, if that looks cleaner.
let me know.

Skimming through them they look very similar and with no context or method documentation I do not find the difference clear. Running low on time so I'll leave this be for now.

ivov · 2023-10-05T08:44:33Z

packages/cli/src/LoadNodesAndCredentials.ts

-
-		await writeStaticJSON('nodes', this.types.nodes);
-		await writeStaticJSON('credentials', this.types.credentials);
+	addPostProcessor(fn: () => Promise<void>) {


I take it we went for this to avoid adding more dependencies.

There are only two post-processors for now, but over time I worry it can become rather hard to keep track of what's getting added and later called when and in what order.

yeah. I couldn't come up with a better way to decouple this processing pipeline. This works for now, but I'm very much open to suggestions.

I'll keep thinking if there's another way but let's not block on this.

packages/cli/src/Server.ts

…endpoints

ivov

~~I'll come back to test manually: community nodes, hot reload, overwrites, etc.~~

Edit: Tested, all fine

packages/cli/package.json

ivov · 2023-10-09T07:47:51Z

packages/cli/src/LoadNodesAndCredentials.ts

+				return filePath;
+			}
+		}
+		return undefined;


Suggested change

return undefined;

removing this makes eslint complain about

Not all code paths return a value

ivov · 2023-10-09T07:51:46Z

packages/cli/src/Server.ts

+				this.frontendService.generateTypes(),
+			);
+			await this.frontendService.generateTypes();


So we have to both register and call generateTypes because LoadNodesAndCredentials.init() was already called during startup at BaseCommand but without this postprocessor? So we registering this postprocessor only for cases where we reload as in with community packages or hot reload?

unfortunately yes. but that also makes sense as LoadNodesAndCredentials.init is called in webhook and worker services as well. It would have been cool to be able to defer postProcessLoaders somehow, but everything I tried so far made this code even more twisted.
I'd definitely like to simplify this, but I'm out of ideas for now.

ivov · 2023-10-09T07:56:18Z

packages/cli/src/config/schema.ts

-		packagesMissing: {
-			// Used to have a persistent list of packages
-			doc: 'Contains a comma separated list of packages that failed to load during startup',
-			format: String,
-			default: '',
-		},


@krynble Is this fine to remove? Or was there a reason this needed to be an env in the first place?

there was never an env variable for this. and this should never have been a config variable either. looking at the code where this was originally added, it looks like we needed a central place to store this string while the application was running, because the code using this flag was scattered across multiple files.
now, this info is centralized in the CommunityPackagesService.

packages/cli/src/services/frontend.service.ts

…endpoints

cypress · 2023-10-09T14:08:52Z

Passing run #2436 ↗︎

0	250	0	0	0

Details:

🌳 refactor-community-endpoints 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️...
Project: n8n	Commit: `5a59e1fefe`
Status: Passed	Duration: 09:11 💡
Started: Oct 9, 2023 1:59 PM	Ended: Oct 9, 2023 2:08 PM

Review all test suite changes for PR #7283 ↗︎

github-actions · 2023-10-09T14:09:03Z

✅ All Cypress E2E specs passed

* master: (42 commits) fix(editor): Make workflow history button available only for dev builds (#7392) refactor: Upgrade to pnpm 8.9 (no-changelog) (#7393) ci: Identify NPM releases as `stable` (no-changelog) fix(editor): Implement canvas zoom UX improvements (#7376) feat(core): Switch binary filesystem mode to nested path structure (#7307) fix(editor): Fix completions for `.json` on quoted node name in Code node (#7382) fix(Notion Node): Handle empty values correctly for Notion selects + multi selects (#7383) feat(editor): Make PDF and Audio binary-data viewable in the UI (#7367) fix(Google Sheets Node): Fix "Maximum call stack size exceeded" error on too many rows (#7384) refactor(core): Refactor nodes loading (no-changelog) (#7283) fix(core): Add an option to enable postgres ssl with default certs (#6889) feat(editor): Workflow history [WIP]- Add restore and clone into new workflow actions (no-changelog) (#7359) fix(HTML Node): Update property fields to not use expressions on drag (#7379) fix: Add role check for upgrade path (#7374) fix(editor): Remove excess margin below run data editor (#7372) fix(Google Drive Trigger Node): Add Shared Drives support (#7369) feat(core): Add Job Summary to Worker response (#7360) feat(Execute Workflow Node): Run once for each item mode (#7289) refactor(core): Move `copyInputItems` to node helpers (no-changelog) (#7299) refactor(core): Create controller for binary data (no-changelog) (#7363) ... # Conflicts: # cypress/e2e/5-ndv.cy.ts

janober · 2023-10-11T13:57:08Z

Got released with [email protected]

Quick follow-up to #7283

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Sep 27, 2023

netroy force-pushed the refactor-community-endpoints branch from c2c2c70 to ac5216c Compare September 27, 2023 20:07

ivov reviewed Sep 29, 2023

View reviewed changes

netroy commented Sep 29, 2023

View reviewed changes

packages/cli/src/LoadNodesAndCredentials.ts Outdated Show resolved Hide resolved

netroy added 3 commits September 29, 2023 16:25

refactor(core): Refactor community-packages endpoints (no-changelog)

52214a8

This also fixes PAY-605

address PR comments and lazy load CommunityPackagesService

95e3530

make CredentialsOverwrites a service, and simplify the dependency tree

49ab975

netroy force-pushed the refactor-community-endpoints branch from ac5216c to 49ab975 Compare September 29, 2023 15:22

netroy requested a review from ivov September 29, 2023 15:25

netroy added 2 commits September 29, 2023 19:28

fix tests

beb788f

block LoadNodesAndCredentials.init in tests

a283e8c

netroy changed the title ~~refactor(core): Refactor community-packages endpoints (no-changelog)~~ refactor(core): Refactor nodes loading (no-changelog) Sep 29, 2023

netroy commented Oct 2, 2023

View reviewed changes

ivov reviewed Oct 5, 2023

View reviewed changes

netroy added 6 commits October 6, 2023 16:19

stop storing packagesMissing in config

7797d4b

Merge remote-tracking branch 'origin/master' into refactor-community-…

5f9cff4

…endpoints

address PR comments

6a7237b

Merge remote-tracking branch 'origin/master' into refactor-community-…

8ad6103

…endpoints

address more comments

8147623

Merge remote-tracking branch 'origin/master' into refactor-community-…

62d1628

…endpoints

netroy requested a review from ivov October 6, 2023 16:02

fix tests

d53b1d7

ivov reviewed Oct 9, 2023

View reviewed changes

netroy added 2 commits October 9, 2023 14:59

remove the generic in FrontendService

4481d15

Merge remote-tracking branch 'origin/master' into refactor-community-…

5a59e1f

…endpoints

netroy requested a review from ivov October 9, 2023 13:00

ivov approved these changes Oct 9, 2023

View reviewed changes

netroy merged commit c5ee06c into master Oct 9, 2023
55 checks passed

netroy deleted the refactor-community-endpoints branch October 9, 2023 14:09

ivov mentioned this pull request Oct 10, 2023

refactor(core): Move frontend settings to service (no-changelog) #7396

Merged

janober added the Released label Oct 11, 2023

ivov added a commit that referenced this pull request Oct 11, 2023

refactor(core): Move frontend settings to service (no-changelog) (#7396)

54e9009

Quick follow-up to #7283

elsmr pushed a commit that referenced this pull request Oct 19, 2023

refactor(core): Move frontend settings to service (no-changelog) (#7396)

d6cc18d

Quick follow-up to #7283

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Refactor nodes loading (no-changelog) #7283

refactor(core): Refactor nodes loading (no-changelog) #7283

netroy commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

ivov left a comment

ivov Sep 29, 2023

codecov bot commented Sep 29, 2023 •

edited

Loading

netroy Oct 2, 2023

netroy Oct 2, 2023

ivov Oct 5, 2023

ivov Oct 5, 2023

netroy Oct 5, 2023

ivov Oct 5, 2023

netroy Oct 6, 2023

ivov Oct 9, 2023

ivov Oct 5, 2023

netroy Oct 6, 2023

ivov Oct 9, 2023

ivov left a comment •

edited

Loading

ivov Oct 9, 2023

netroy Oct 9, 2023

ivov Oct 9, 2023

netroy Oct 9, 2023

ivov Oct 9, 2023

netroy Oct 9, 2023

cypress bot commented Oct 9, 2023

github-actions bot commented Oct 9, 2023

janober commented Oct 11, 2023

		@@ -50,20 +40,20 @@ export class LoadNodesAndCredentials implements INodesAndCredentials {

		includeNodes = config.getEnv('nodes.include');

		credentialTypes: ICredentialTypes;


		LoggerProxy.init(getLogger());
		LoggerProxy.init(mock<ILogger>());

refactor(core): Refactor nodes loading (no-changelog) #7283

refactor(core): Refactor nodes loading (no-changelog) #7283

Conversation

netroy commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

ivov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 29, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov left a comment • edited Loading

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

cypress bot commented Oct 9, 2023

Passing run #2436 ↗︎

Review all test suite changes for PR #7283 ↗︎

github-actions bot commented Oct 9, 2023

janober commented Oct 11, 2023

codecov bot commented Sep 29, 2023 •

edited

Loading

ivov left a comment •

edited

Loading