codemirror / codemirror5

In-browser code editor (version 5, legacy)
http://codemirror.net/5/
MIT License
26.77k stars 4.96k forks source link

Architecture update suggestion #5438

Closed krzysztof-miemiec closed 6 years ago

krzysztof-miemiec commented 6 years ago

CodeMirror is actively maintained, yet it's architecture looks as it's stuck in a JS world from a few years ago. I tried to integrate it with my project (Electron + Typescript + Webpack + React) and had a really hard time figuring out why it doesn't work with react-codemirror2.

First of all, for people having a similar issue, I created a separate file codemirror.util.tsx

import 'codemirror/lib/codemirror.css';
import 'codemirror/theme/idea.css';

const CodeMirror = (window as any).CodeMirror = (global as any).CodeMirror = require('CodeMirror');

// Here I extracted the contents of mode that I was interested in (sql)
const sql = (CodeMirror) => {
  CodeMirror.defineMode('sql', function (config, parserConfig) {
    'use strict';

    const client = parserConfig.client || {},
    // ...
 //...
};

sql(CodeMirror);

It seems that require('codemirror) doesn't work well in pure JS projects - I couldn't simply include a mode, css and react-codemirror2 component, because mode and component didn't use a single instance of codemirror, plus I don't have control over <script> tags beacuse of electron-webpack layer.

I'd really like to be able to manually register a mode out of the box, like:

import CodeMirror from 'codemirror';
import { sql } from 'codemirror/modes/sql';
import { UnControlled } from 'react-codemirror2';

sql(CodeMirror); // This is the most reasonable way, as `CodeMirror.defineMode(sql)` won't work for modes, because it also uses `defineMIME`

export const CodeInput = () => (
  <UnControlled
            className={classes.input}
            value={text}
            instance={CodeMirror}
            options={{
              mode: 'text/x-pgsql',
              theme: 'idea',
              lineNumbers: true,
              tabSize: 2,
            }}
          />
);

This change could possibly be introduced in next major version? I can make a PR with modes changes.

marijnh commented 6 years ago

because mode and component didn't use a single instance of codemirror

That's an issue with your bundler/module loader, not CodeMirror. The various files require the main library through relative paths, which the loader should unify to load only a single instance unless you have multiple instances of the library in your node_modules. You didn't mention which bundler you're using, but this is a problem that's come up before.

We're planning a full rewrite that will completely change how any of this works, but in the meantime, the current approach should be workable enough.

krzysztof-miemiec commented 6 years ago

It never happened with any other library I worked with, so I'd count it as a CodeMirror issue. 😕 Nevertheless, I'll be waiting for a rewrite!