-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
feat: migrate editor-ui to Vite.js and various DX improvements (N8N-2277) #4061
Conversation
…json imports config.
⚡ Serve CSS files for Vite migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Cannot approve because of outstanding issues that need to be resolved
import { WorkflowActivationError } from './WorkflowActivationError'; | ||
import { WorkflowOperationError } from './WorkflowErrors'; | ||
import { NodeApiError, NodeOperationError } from './NodeErrors'; | ||
import type { Workflow } from './Workflow'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is type
needed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a typescript feature that allows you to import declarations only, comes in handy when you need to remove circular references and imports.
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should all be type imports.
@@ -1,7 +1,5 @@ | |||
@import "./n8n-theme-variables"; | |||
|
|||
@import "~n8n-design-system/theme/dist/index.css"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how are we importing this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're importing it directly in design-system/src/main.js
. Doing so allows the build system to split into chunks and write font/asset paths properly.
Found this error on 5678 when switching locale:
To reproduce, create |
@ivov Only the first few loads will be slower until Vite creates the dependencies graph and caches it. Subsequent starts are much faster than webpack. At least that's how it is for me. |
Good catch, working on fixing it! |
Looks like we both contributed a bit to this 😅. Fixed! |
readIndexFile = readIndexFile.replace(/\/favicon.ico/g, `${n8nPath}favicon.ico`); | ||
|
||
const cssPath = pathJoin(pathDirname(editorUiPath), 'dist', '**/*.css'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change means that we'll be keeping a copy of all JS and CSS files in the memory, and increasing the overall memory usage of every instance. Also, this makes gzipping these resources more difficult, which is not really an issue as we are currently not compressing any of our assets (from what I can tell).
However. this isn't a blocker. I can take care of optimizing these and setting up compression in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, Adi. I've crated a task for it: https://linear.app/n8n/issue/N8N-4739
I'm seeing this issue on my staging instance. every reload takes significantly longer than it used to. even though none of the http requests seem slow. |
Interesting. I see now. I thought @ivov was referring to initial page load. I'll try to do something about this. |
@@ -58,8 +58,7 @@ const module: Module<INodeTypesState, IRootState> = { | |||
}, | |||
mutations: { | |||
setNodeTypes(state, newNodeTypes: INodeTypeDescription[]) { | |||
// tslint:disable-next-line:no-any | |||
const nodeTypes = newNodeTypes.reduce((acc: any, newNodeType) => { | |||
const nodeTypes = newNodeTypes.reduce((acc: Record<string, Record<string, INodeTypeDescription>>, newNodeType) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The type for the accumulator is provided as a generic to reduce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today I learned 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍻
Got released with |
…277) (n8n-io#4061) * feat: Added vite.js dependencies. * chore: Removed tests folder to follow same structure as design-system * chore: Removed unused testing config. * chore: Created vite.js index.html * refactor: Updated scss structure and imports. * refactor: Updated workflow building. * fix: Cleared up all workflow dependency cycles. Added proper package.json imports config. * feat: Got a working build using Vite. Need to fix issues next. * fix: Progress! Getting process.env error. * fix: Changed process.env to import.meta.env. * fix: Fixed circular imports that used require(). Fixed monaco editor. * chore: Removed commented code. * chore: Cleaned up package.json * feat: Made necessary changes to replace base path in css files. * feat: Serve CSS files for `editor-ui` Vite migration (n8n-io#4069) :zap: Serve CSS files for Vite migration * chore: Fixed package-lock.json. * fix: Fixed build after centralized tsconfig update. * fix: Removed lodash-es replacement. * fix: Commented out vitest test command. * style: Fixed linting issues. * fix: Added lodash-es hotfix back. * chore: Updated package-lock.json * refactor: Renamed all n8n scss variables to no longer be defined as private. * feat(editor): add application-wide el-button replacement. * fix(editor): Fix import in page alert after merge. * chore(editor): update package-lock.json. * fix: Case sensitive lodash-es replacement for vue-agile. * fix: add alias for lodash-es camelcase import. * fix: add patch-package support for fixing quill * feat: add patch-package on postinstall * fix: update quill patch path. * refactor: rename quill patch * fix: update quill version. * fix: update quill patch * fix: fix linting rules after installing eslint in design-system * fix: update date picker button to have primary color * test: update callout component snapshots * fix(editor): fix linting issues in editor after enabling eslint * fix(cli): add /assets/* to auth ignore endpoints in server * chore: update package-lock.json * chore: update package-lock.json * fix(editor): fix linting issues * feat: add vite-legacy support * fix: update workflow package interface imports to type imports. * chore: update package-lock.json * fix(editor) fix importing translations other than english * fix(editor): remove test command until vitest is added * fix: increase memory allocation for vite build * fix: add patch-package patches to n8n-custom docker build * fix: add performance and load time improvements * fix: add proper typing to setNodeType * chore: update package-lock.json * style: use generic type for reduce in setNodeType Co-authored-by: Iván Ovejero <[email protected]>
No description provided.