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

#1640 Remove react-codemirror2 and upgrade to codemirror 6 #1641

Merged
merged 17 commits into from
Feb 20, 2024

Conversation

nmgokhale
Copy link
Member

@nmgokhale nmgokhale commented Dec 4, 2023

Fixes: #1640

Release notes documentation -

Major version upgrade from Codemirror 5.x to Codemirror 6.x. This affects the code editor in Expression control.

Screenshot 2024-02-08 at 3 58 25 PM

Gruntfile changes -
We're No longer exporting lib/codemirror.css and addon/hint/show-hint.css files.

If your application is using following grunt config, please remove this grunt config.

codeMirror: {
	files: [{
		expand: true,
		flatten: false,
		cwd: "./node_modules/codemirror/",
		src: ["lib/codemirror.css", "addon/hint/show-hint.css"],
		dest: ".build/css/codemirror"
	}]
},

Also remove following stylesheets from your application's index.js file -

<link rel="stylesheet" href={`/canvas${getHashedResource("/css/codemirror/addon/hint/show-hint.css")}`} />
<link rel="stylesheet" href={`/canvas${getHashedResource("/css/codemirror/lib/codemirror.css")}`} />

CSS Changes -
Since we're no longer exporting lib/codemirror.css file, if you create a new Codemirror 6.x instance in your application, elyra-canvas codemirror CSS from this file will no longer be imported into your classnames. Please ensure all custom CSS overrides works fine in your application.

Codemirror 6.x creates a rather different DOM structure for the editor. If you're writing custom CSS for the editor, you'll probably have to change it a bit. Class names roughly correspond like this:

react-codemirror2 → elyra-CodeMirror
CodeMirror → cm-editor
CodeMirror-line → cm-line
CodeMirror-scroll → cm-scroller
CodeMirror-sizer → cm-content
CodeMirror-focused → cm-focused
CodeMirror-gutters → cm-gutters
CodeMirror-gutter → cm-gutter
CodeMirror-gutter-elt → cm-gutterElement

Please make any other changes in class names as required after inspecting "Elements" tab in the browser.

Autocompletions -
Expression control supports following languages -

  • Python
  • R
  • SQL
  • JavaScript
  • CLEM

Autocompletions will start showing up as soon as you start typing in the code editor. They can also show up using ctrl + space key combination. Autocompletions will show language specific keywords, custom keywords specified here, and field names from the expression builder table.

Screenshot 2024-02-08 at 4 01 19 PM

Jest tests failures -
If you notice several jest unit tests are failing with this error -

● Test suite failed to run

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".
    ...

    Details:

    @elyra/canvas/canvas_modules/common-canvas/node_modules/lezer-r/dist/highlight.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import { styleTags, tags as t } from "@lezer/highlight";
                                                                                             ^^^^^^

    SyntaxError: Cannot use import statement outside a module

Please add this config in your package.json file to fix jest tests -

"jest": {
    "transformIgnorePatterns": [
      "node_modules/@elyra/canvas/node_modules/(?!lezer-r)"
    ]
  }

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@nmgokhale nmgokhale marked this pull request as draft December 4, 2023 07:05
@nmgokhale nmgokhale force-pushed the codemirror6-upgrade branch 2 times, most recently from ec1941a to aa0784e Compare January 23, 2024 23:59
@nmgokhale nmgokhale self-assigned this Feb 5, 2024
@@ -111,7 +119,7 @@
},
"jest": {
"transformIgnorePatterns": [
"node_modules/(?!d3-*)"
"/node_modules/(?!(lezer-r|d3|d3-array|d3-axis|d3-brush|d3-chord|d3-color|d3-contour|d3-delaunay|d3-dispatch|d3-drag|d3-dsv|d3-ease|d3-fetch|d3-force|d3-format|d3-geo|d3-hierarchy|d3-interpolate|d3-path|d3-polygon|d3-quadtree|d3-random|d3-scale|d3-scale-chromatic|d3-selection|d3-shape|d3-time|d3-time-format|d3-timer|d3-transition|d3-zoom)/)"
Copy link
Member Author

Choose a reason for hiding this comment

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

d3-* in following config didn't work. So I had to specify all d3 packages in the config.

transformIgnorePatterns": [
      "/node_modules/(?!(d3-*|lezer-r)/)"
],

@nmgokhale nmgokhale force-pushed the codemirror6-upgrade branch from 08f6922 to 3c92086 Compare February 9, 2024 00:04
@nmgokhale nmgokhale marked this pull request as ready for review February 9, 2024 00:04
let cursor = this.editor.getCursor();
const somethingSelected = this.editor.viewState.state.selection.ranges.some((r) => !r.empty);
let cursor = this.editor.viewState.state.selection.main.head;
if (cursor === 0 && !somethingSelected) { // TODO: Doesn't work when I explicitly set the cursor to 0
Copy link
Member Author

Choose a reason for hiding this comment

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

When nothing is selected, cursor is set to the end of the line. But this if loop doesn't work when I explicitly set the cursor to line 0, char 0 -

Screen.Recording.2024-02-08.at.4.08.46.PM.mov

Copy link
Member

@matthoward366 matthoward366 left a comment

Choose a reason for hiding this comment

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

When accessing the state is this documented? I'm thinking we need to either add 1 method to get the state to make sure it returns something or add optional chaining.

@@ -55,7 +63,7 @@
"@rollup/plugin-babel": "5.3.0",
"@rollup/plugin-commonjs": "21.0.1",
"@rollup/plugin-json": "4.1.0",
"@rollup/plugin-node-resolve": "13.0.6",
"@rollup/plugin-node-resolve": "^13.0.6",
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you changed the devDependencies? These should stay static since consuming applications wouldn't install them.

}

onChange(newValue) {
const value = (typeof newValue === "string") ? newValue : newValue.toString();
let cursor = this.editor.getCursor();
const somethingSelected = this.editor.viewState.state.selection.ranges.some((r) => !r.empty);
let cursor = this.editor.viewState.state.selection.main.head;
Copy link
Member

Choose a reason for hiding this comment

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

Are both of these internal to CodeMirror? We probably should add optional chaining in case CodeMirror changes the code and these objects don't exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add a new method which will return this.editor?.viewState?.state

Getting the state.doc, state.selection, state.replaceSelection is documented in this migration guide - https://codemirror.net/docs/migration/


const pxPerChar = 8.5;
const pxPerLine = 26;
const defaultCharPerLine = 30;
const maxLineHeight = 15 * pxPerLine; // 20 lines
const minLineHeight = 4 * pxPerLine; // 4 lines

const editable = new Compartment; // eslint-disable-line
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you define this here and not as a class variable?

@@ -47,7 +47,7 @@
"carbon-components": "10.56.0",
"carbon-components-react": "7.56.0",
"carbon-icons": "7.0.7",
"codemirror": "5.58.2",
"codemirror": "6.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed now. This should come from common-canvas.

Signed-off-by: Neha Gokhale <[email protected]>
const somethingSelected = this.editor.viewState.state.selection.ranges.some((r) => !r.empty);
let cursor = this.editor.viewState.state.selection.main.head;
const somethingSelected = this.getCodemirrorState()?.selection.ranges.some((r) => !r.empty);
let cursor = this.getCodemirrorState()?.selection.main.head;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really help protect us. It would have to be this.getCodemirrorState()?.selection?.main?.head

Anything that has methods would have to change the object first, then call the method.

Is everything your using documented? If so, maybe we're safe to use it without optional chaining.

Copy link
Member Author

Choose a reason for hiding this comment

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

Everything after this.getCodemirrorState()? is documented in migration guide and reference manual.

Copy link
Member

@matthoward366 matthoward366 left a comment

Choose a reason for hiding this comment

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

Looks good

@nmgokhale nmgokhale merged commit e913d9a into elyra-ai:main Feb 20, 2024
3 checks passed
@nmgokhale nmgokhale deleted the codemirror6-upgrade branch February 20, 2024 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove react-codemirror2 and upgrade to codemirror 6
2 participants