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

feat(editor): New Code editor based on the TypeScript language service #12285

Open
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

elsmr
Copy link
Member

@elsmr elsmr commented Dec 18, 2024

Summary

TODO: needs tests

  • TypeScript autocomplete
  • TypeScript linting
  • TypeScript hover tips
  • Search & replace
  • New keyboard shortcuts based on the VSCode keymap
  • Auto-formatting using prettier (Alt+Shift+F)
  • Remember folded regions and history after refresh
  • Multi cursor
  • Type function in the Code node using JSDoc types
  • Web Worker Architecture
  • Drag/drop in all Code node modes
  • Indentation markers
image image

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/NODE-1466/overhaul-code-node-p0

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@elsmr elsmr added the tests-needed This PR needs additional tests label Dec 18, 2024
@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Dec 18, 2024
dana-gill
dana-gill previously approved these changes Dec 19, 2024
Copy link
Contributor

@dana-gill dana-gill left a comment

Choose a reason for hiding this comment

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

A comment about the lockfile, but if the packages were added via package.json and not pnpm, then lgtm 🎉

@@ -1345,6 +1345,9 @@ importers:
'@codemirror/lint':
specifier: ^6.8.0
version: 6.8.0
'@codemirror/search':
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been burned by the lockfile before so I just wanted to double check that you made the package additions using package.json and not using pnpm i as mentioned here, right? If that's the case, lgtm 🥳 Amazing change and really cool feature!

Copy link
Contributor

✅ No visual regressions found.

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

Copy link

cypress bot commented Dec 19, 2024

n8n    Run #8421

Run Properties:  status check failed Failed #8421  •  git commit 9dceb02ec3: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 elsmr 🗃️ e2e/*
Project n8n
Branch Review node-1466-overhaul-code-node-p0
Run status status check failed Failed #8421
Run duration 06m 33s
Commit git commit 9dceb02ec3: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 elsmr 🗃️ e2e/*
Committer Elias Meire
View all properties for this run ↗︎

Test results
Tests that failed  Failures 16
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 300
View all changes introduced in this branch ↗︎

Tests for review

Failed  5-ndv.cy.ts • 2 failed tests

View Output Video

Test Artifacts
NDV > should allow editing code in fullscreen in the Code node Test Replay Screenshots Video
NDV > should properly show node execution indicator Test Replay Screenshots Video
Failed  12-canvas.cy.ts • 2 failed tests

View Output Video

Test Artifacts
Canvas Node Manipulation and Navigation > should add nodes and check execution success Test Replay Screenshots Video
Canvas Node Manipulation and Navigation > should preserve connections after rename & node-view switch Test Replay Screenshots Video
Failed  12-canvas-actions.cy.ts • 1 failed test

View Output Video

Test Artifacts
Canvas Actions > Node hover actions > should execute node Test Replay Screenshots Video
Failed  45-ai-assistant.cy.ts • 2 failed tests

View Output Video

Test Artifacts
AI Assistant::enabled > should apply code diff to code node Test Replay Screenshots Video
AI Assistant::enabled > Should ignore node execution success and error messages after the node run successfully once Test Replay Screenshots Video
Failed  6-code-node.cy.ts • 9 failed tests

View Output Video

Test Artifacts
Code node > Code editor > should execute the placeholder successfully in both modes Test Replay Screenshots Video
Code node > Code editor > should show lint errors in `runOnceForAllItems` mode Test Replay Screenshots Video
Code node > Code editor > should show lint errors in `runOnceForEachItem` mode Test Replay Screenshots Video
... > generate code button should have correct state & tooltips Test Replay Screenshots Video
... > should send correct schema and replace code Test Replay Screenshots Video
... > should show error based on status code 400 Test Replay Screenshots Video
... > should show error based on status code 413 Test Replay Screenshots Video
... > should show error based on status code 429 Test Replay Screenshots Video
... > should show error based on status code 500 Test Replay Screenshots Video

The first 5 failed specs are shown, see all 50 specs in Cypress Cloud.

Flakiness  19-execution.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Execution > connections should be colored differently for pinned data > when executing a node Test Replay Screenshots Video
Flakiness  14-mapping.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Data mapping > maps expressions from json view Test Replay Screenshots Video

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 tests-needed This PR needs additional tests ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants