Skip to content

Commit

Permalink
CodeMirror lint: stop patching global lint (#26013)
Browse files Browse the repository at this point in the history
## Summary & Motivation

We currently override CodeMirror's global lint addon with a custom
implementation that conflicts with the original addon. This
incompatibility surfaced while developing the new asset selection syntax
input; specifically, loading the launchpad and then accessing the new
syntax input triggers JavaScript errors due to the overridden lint
behavior.

To fix this issue, this PR stops patching the global lint addon and
instead uses a separate patchedLint namespace. By doing so, our custom
lint functionality is isolated to specific editor instances, allowing
the original lint addon to operate correctly in other editors without
interference.


## How I Tested These Changes

Use the launchpad and see that it lints and surfaces errors correctly by
starting with a valid config, adding invalid stuff, and then making it
valid again and making sure we lint each step along the way.

<img width="1089" alt="Screenshot 2024-11-19 at 3 41 55 PM"
src="https://github.com/user-attachments/assets/c58e9637-42cf-4c28-9f75-5b5f50d1dce7">
<img width="908" alt="Screenshot 2024-11-19 at 3 42 28 PM"
src="https://github.com/user-attachments/assets/74a66579-2a1f-487a-bfa5-a817793bd102">
<img width="784" alt="Screenshot 2024-11-19 at 3 42 20 PM"
src="https://github.com/user-attachments/assets/b895f385-a0ad-42b4-bc8f-47944dc2e284">
  • Loading branch information
salazarm authored Nov 20, 2024
1 parent 7067c5f commit 21818b6
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ interface ConfigEditorProps {

const AUTO_COMPLETE_AFTER_KEY = /^[a-zA-Z0-9_@(]$/;
const performLint = debounce((editor: any) => {
editor.performLint();
editor.performPatchedLint();
}, 1000);

const performInitialPass = (
Expand Down Expand Up @@ -127,7 +127,7 @@ export const NewConfigEditor = forwardRef<ConfigEditorHandle, ConfigEditorProps>
smartIndent: true,
showCursorWhenSelecting: true,
lintOnChange: false,
lint: {
patchedLint: {
checkConfig,
lintOnChange: false,
onUpdateLinting: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ function parseOptions(_cm, options) {
}

function clearMarks(cm) {
const state = cm.state.lint;
const state = cm.state.patchedLint;
if (state.hasGutter) {
cm.clearGutter(GUTTER_ID);
}
Expand Down Expand Up @@ -146,7 +146,7 @@ function annotationTooltip(ann) {

function updateLinting(cm, annotationsNotSorted) {
clearMarks(cm);
const state = cm.state.lint,
const state = cm.state.patchedLint,
options = state.options;

const annotations = groupByLine(annotationsNotSorted);
Expand Down Expand Up @@ -199,7 +199,7 @@ function updateLinting(cm, annotationsNotSorted) {
}

function lintAsync(cm, getAnnotations, passOptions) {
const state = cm.state.lint;
const state = cm.state.patchedLint;
let id = ++state.waitingFor;
function abort() {
id = -1;
Expand All @@ -226,7 +226,7 @@ function lintAsync(cm, getAnnotations, passOptions) {
}

function startLinting(cm) {
const state = cm.state.lint,
const state = cm.state.patchedLint,
options = state.options;
/*
* Passing rules in `options` property prevents JSHint (and other linters) from complaining
Expand Down Expand Up @@ -259,7 +259,7 @@ function startLinting(cm) {
}

function onChange(cm) {
const state = cm.state.lint;
const state = cm.state.patchedLint;
if (!state) {
return;
}
Expand Down Expand Up @@ -328,15 +328,15 @@ function LintState(cm, options, hasGutter) {
}

export const patchLint = () => {
CodeMirror.defineOption('lint', false, function (cm, val, old) {
CodeMirror.defineOption('patchedLint', false, function (cm, val, old) {
if (old && old !== CodeMirror.Init) {
clearMarks(cm);
if (cm.state.lint.options.lintOnChange !== false) {
if (cm.state.patchedLint.options.lintOnChange !== false) {
cm.off('change', onChange);
}
CodeMirror.off(cm.getWrapperElement(), 'mouseover', cm.state.lint.onMouseOver);
clearTimeout(cm.state.lint.timeout);
delete cm.state.lint;
CodeMirror.off(cm.getWrapperElement(), 'mouseover', cm.state.patchedLint.onMouseOver);
clearTimeout(cm.state.patchedLint.timeout);
delete cm.state.patchedLint;
}

if (val) {
Expand All @@ -347,7 +347,11 @@ export const patchLint = () => {
hasLintGutter = true;
}
}
const state = (cm.state.lint = new LintState(cm, parseOptions(cm, val), hasLintGutter));
const state = (cm.state.patchedLint = new LintState(
cm,
parseOptions(cm, val),
hasLintGutter,
));
if (state.options.lintOnChange !== false) {
cm.on('change', onChange);
}
Expand All @@ -357,8 +361,8 @@ export const patchLint = () => {
}
});

CodeMirror.defineExtension('performLint', function () {
if (this.state.lint) {
CodeMirror.defineExtension('performPatchedLint', function () {
if (this.state.patchedLint) {
startLinting(this);
}
});
Expand Down

1 comment on commit 21818b6

@github-actions
Copy link

Choose a reason for hiding this comment

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

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-o8bbzxhbk-elementl.vercel.app

Built with commit 21818b6.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.