n8n icon indicating copy to clipboard operation
n8n copied to clipboard

N8N-2827 speed up nodes base

Open alexgrozav opened this issue 2 years ago • 5 comments

Issue

Building thousands of files in the nodes-base package is slow. Attempt switching to vite or esbuild.

Findings

Switching to Vite.js

Vite.js is focused on creating application bundles and packages. This means that there should be only a few entry points. The nodes-base package has hundreds of entry points that need to be compiled individually. Even though Vite is super fast, it's not what it is built for.

Vite.js build time

npm run build 59.20s user 2.20s system 178% cpu 18.339 total

Switching to esbuild

Even after switching to esbuild, I was only able to shave off a few seconds from the build because the .d.ts still need to be generated using tsc. Esbuild is fast, but it simply ignores typings when compiling https://github.com/evanw/esbuild/issues/95#issuecomment-626069971.

Proposed solution

Esbuild is able to compile the whole project in under 1s without type definitions. The only typings used outside of development are found in the src folder. Instead of generating typescript definition files for each node, we can generate them only for the files inside the src folder.

This dramatically reduces build time.

Old build time

npm run build:old  22.30s user 1.10s system 176% cpu 13.229 total

New build time

npm run build  3.85s user 0.59s system 125% cpu 3.549 total

Benchmarks run on a 16" MacBook Pro with M1 Pro

If still needed, we could add a script for generating the d.ts for nodes when creating a new release.

Alternatively, could let the build command build all d.ts files and then have a build:fast to rebuild .ts using esbuild, and copy assets only.

What do you think?

alexgrozav avatar Apr 08 '22 14:04 alexgrozav

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 08 '22 14:04 CLAassistant

Sorry for the delay! Re-review:

  1. n8n fails to initialize - unable to load some esbuilt nodes:

git reset --hard; git clean -dffx
lerna bootstrap --hoist; npm run build
npm run start


> [email protected] start
> run-script-os

> [email protected] start:default
> cd packages/cli/bin && ./n8n

Initializing n8n process
 ›   Error: There was an error: Cannot find module './v1/actions/router'
 ›   Require stack:
 ›   - /Users/ivov/Development/n8n/packages/nodes-base/dist/nodes/BambooHr/BambooHr.node.js
 ›   - /Users/ivov/Development/n8n/packages/cli/dist/src/LoadNodesAndCredentials.js
 ›   - /Users/ivov/Development/n8n/packages/cli/dist/src/index.js
 ›   - /Users/ivov/Development/n8n/packages/cli/dist/commands/execute.js
 ›   - /Users/ivov/Development/n8n/node_modules/@oclif/config/lib/plugin.js
 ›   - /Users/ivov/Development/n8n/node_modules/@oclif/config/lib/config.js
 ›   - /Users/ivov/Development/n8n/node_modules/@oclif/config/lib/index.js
 ›   - /Users/ivov/Development/n8n/node_modules/@oclif/command/lib/command.js
 ›   - /Users/ivov/Development/n8n/node_modules/@oclif/command/lib/index.js
 ›   - /Users/ivov/Development/n8n/packages/cli/bin/n8n

Removing Bamboo HR triggers same error for Cisco Webex and then Elasticsearch.

The issue seems to have been tiny-glob not going deeply enough into the directory tree. Switched to fast-glob to match cli.

  1. .d.ts are still being included, becoming .d.js in /dist. The glob in build.mjs is not excluding them: console.log(tsFiles.filter(f => f.endsWith('.d.ts')));

Fixed. Fast glob supports negation glob.

  1. The existing TS build process emits CJS with the use strict directive; esbuild is not doing this, but supports a workaround.

Added 'use strict'; via banner.js.

  1. Switching to a non-English locale causes the build to fail:
export N8N_DEFAULT_LOCALE=de
cd packages/nodes-base; npm run build
node:events:368
      throw er; // Unhandled 'error' event
      ^

Error: ENOENT: no such file or directory, open '/Users/ivov/Development/n8n/packages/nodes-base/dist/nodes/headers.js'
Emitted 'error' event on Domain instance at:
    at emit (node:internal/process/promises:134:35)
    at processPromiseRejections (node:internal/process/promises:242:25)
    at processTicksAndRejections (node:internal/process/task_queues:97:32) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/Users/ivov/Development/n8n/packages/nodes-base/dist/nodes/headers.js'
}
ERROR: "build:assets" exited with 1.

This issue is absent from master. The build stages are now parallel, so I expect the attempt to write to the headers path finishes first, failing to find the dist dir.

Edit: 4.5. Should we restore type-checking to the build process with --noEmit?

Fixed. Ensured folder creation upon build. Extracted cleanup phase to start before parallel execution (duh! 😰).

Minor details

  1. '\n[esbuild] Watching for changes...' is shown both for watch and for build, instead of just for watch. You can use the watch.onRebuild arg, refer to the second watch mode example:
require('esbuild').build({
  entryPoints: ['app.js'],
  outfile: 'out.js',
  bundle: true,
  watch: {
    onRebuild(error, result) {
      if (error) console.error('watch build failed:', error)
      else console.log('watch build succeeded:', result)
    },
  },
}).then(result => {
  console.log('watching...')
})

Added proper build message for build mode and watch mode.

  1. Instead of removing the existing watch script reference in package.json, we should keep it as watch:old just like the old build script. In a few weeks if there are no issues, we can remove both.
  1. After deletion of watch.mjs, shelljs is now only used for rm -rf. We already have rimraf, so shelljs might not be needed.

Removed shelljs.

  1. The only JSON files in /nodes-base/nodes and /nodes-base/credentials are codex files .node.json and potentially translation files. Both kinds are required cross-package from /cli, i.e. they are not directly imported inside /nodes-base, so I expect that nodes/**/*.json and credentials/translations/**/*.json should be removable from tsconfig.json without import issues.
find ./packages/nodes-base -type f -name \*.json ! -name \*.node.json

Removed irrelevant files from tsconfig.json.

  1. (Nitpick) Naming of tsconfig.build.json - the child only takes care of building /src, but both parent and child are responsible for the entire build, which makes the .build. infix for /src potentially confusing. This will become moot once we move src into /cli and skip declaration generation altogether.

Renamed it to tsconfig.dts.json. Does this make more sense? How would you name it?

  1. (Out of scope, tech debt.) We should eventually explore the esbuild minify and tree-shaking flags.

Definitely! I'll create a task for it after this one gets merged.

alexgrozav avatar May 18 '22 12:05 alexgrozav

Thanks for addressing my comments :+1:

  1. .d.ts are still being included, becoming .d.js in /dist. Example: packages/nodes-base/nodes/ActionNetwork/types.d.js A sourcemap is also being generated for the transpiled type def file: types.d.js.map
  2. The esbuild dist size is 32.2 MB, the tsc dist was 23.9 MB. Helpers like __toCommonJS are repeated in every file in every node. Is this size increase accounted for and tolerable? More info: https://github.com/evanw/esbuild/issues/1984
  3. Typechecking is no longer part of the build process. Is this intentional?
  4. Re: naming for tsconfig.dts.json, I was thinking of tsconfig.src.json, since /src is the only dir that it targets.

ivov avatar May 23 '22 09:05 ivov

Thanks for addressing my comments 👍

  1. .d.ts are still being included, becoming .d.js in /dist. Example: packages/nodes-base/nodes/ActionNetwork/types.d.js A sourcemap is also being generated for the transpiled type def file: types.d.js.map

Looks like the fast-glob block pattern is not working properly. Filtered using JS.

  1. The esbuild dist size is 32.2 MB, the tsc dist was 23.9 MB. Helpers like __toCommonJS are repeated in every file in every node. Is this size increase accounted for and tolerable? More info: QUESTION: best way use esbuild to bundle TypeScript? evanw/esbuild#1984

Not much I can do about this, unfortunately.

  1. Typechecking is no longer part of the build process. Is this intentional?
  • Added typechecking back when running npm run build.
  1. Re: naming for tsconfig.dts.json, I was thinking of tsconfig.src.json, since /src is the only dir that it targets.

  • Added npm run build:fast which skips type checking and dramatically reduces build time
  • I found a way to speed tsc up.
    • Apparently, if you don't refer a project when running it from the command line, you're skipping a lot of boilerplate code. This resulted in a 2s increase.
    • More than that, if you add --skipLibCheck, you shave off another 3s from the build time at the cost of ignoring d.ts file dependencies with errors.

npm run build  16.37s user 1.51s system 235% cpu 7.577 total
npm run build:fast  1.35s user 0.82s system 139% cpu 1.561 total
npm run build:old  21.49s user 0.99s system 180% cpu 11.919 total

alexgrozav avatar Jun 03 '22 11:06 alexgrozav

Update:

  • Added Turborepo integration with parallel build commands
  • Speed up is now 3x for overall build command (85s to 27s)
  • Build caching support via Turborepo
  • Parallelized (uncached) dev support via Turborepo
  • Moved NodeVersionedType to workflow
  • Removed build:fast script
  • Removed tsconfig.dts.json
  • Overhead is now nodes-base and editor-ui, with longest running build command, but parallelization helps a lot

alexgrozav avatar Jun 17 '22 12:06 alexgrozav