search

mizzao / meteor-sharejs

Meteor smart package for transparently adding ShareJS editors to an app
MIT License
225 stars 53 forks source link

Themes and addons in CodeMirror #73

Open edemaine opened 8 years ago

edemaine commented 8 years ago

I tried setting editor.setOption 'theme', 'blackboard' but the colors didn't change. From the CodeMirror docs, it looks like I need to also manually include the relevant CSS (theme/blackboard.css). Similarly, add-on loading seems to be manually by including the relevant JavaScript. (With Ace, this inclusion was automatic.) Should I be adding <script> and <link> tags to my Meteor code to do this?

edemaine commented 8 years ago

I tried adding this JavaScript to my project, but it didn't load... I'm probably doing something wrong.

<head>
<script src="/packages/mizzao_sharejs-codemirror/codemirror/addon/selection/active-line.js"></script>
</head>
edemaine commented 8 years ago

Hmm, .meteor/local/build/programs/web.browser/packages/ has a mizzao_sharejs-codemirror.js (which seems to include a few add-ons, but not many), but no mizzao_sharejs-codemirror directory (whereas Ace gets a directory mizzao_sharejs-ace and a js file mizzao_sharejs-ace.js).

So I take it it's currently impossible to use themes and any of the other add-ons (unlike Ace). Do you think they could be added to the mizzao:sharejs-codemirror package (via api.addAssets like Ace)?

mizzao commented 8 years ago

CodeMirror support was added in #27 and was never really first class (I started with Ace). It would probably be good to reconsider how the files are loaded.

DavidSichau commented 8 years ago

The difference is that for ace all code is loaded as static asset and for codemirror this is currently not done, only the required files are imported.

Maybe we should also switch for codemirror to the same behaviour?

mizzao commented 8 years ago

Yeah, that sounds like a good idea. I just did a quick and dirty integration of CodeMirror so it would be good to do it correctly :)

edemaine commented 8 years ago

Seems reasonable. The alternative would be to add some way of triggering the dynamic loading of desired modules by some user specification. But there aren't a ton of standard modules, so loading them all is probably not horrible (and certainly simpler).

DavidSichau commented 8 years ago

As static assets they are only loaded if an user requires them, so this is not a big issue. However then the user is able to load them.

I will include this change in the pull request tomorrow and publish it if there are no issues.

edemaine commented 8 years ago

Oh, I see. Sounds perfect! Looking forward to switching to CM in my app.

edemaine commented 8 years ago

I'm probably missing something obvious, or there's a problem with some package.js. When I meteor update I got:

Changes to your project's package version selections from updating package
versions:

mizzao:sharejs      upgraded from 0.8.0 to 0.9.0
mizzao:sharejs-ace  upgraded from 1.3.0 to 1.4.0

The following top-level dependencies were not updated to the very latest
version available:
 * mizzao:sharejs-codemirror 4.12.1 (5.14.2 is available)

meteor update mizzao:sharejs-codemirror produces:

The specified packages are at their latest compatible versions.
DavidSichau commented 8 years ago

It seems to be related to this: https://github.com/meteor/meteor/issues/5270

Maybe you have another package requiring sharejs-codemirror

Could you try to manually update the package by changing .meteor/versions

edemaine commented 8 years ago

That helped, thanks! I added @5.14.2 to mizzao:sharejs-codemirror and forced an update with meteor update --allow-incompatible-update; then meteor crashed, but with a useful error message, pointing at Meteor package djedi:sanitize-html-client as the offender. Not sure exactly how that interacts with codemirror, but replacing that package with a modern npm dependency on sanitize-html caused everything to work!

ol-c commented 8 years ago

Hey, thanks for the work guys! How do you recommend we import assets like language modes? I can load the theme source in my browser with a path like:

/packages/mizzao_sharejs-codemirror/.npm/package/node_modules/codemirror/mode/javascript/javascript.js

I would like to import mode files on the client, but I keep getting "Cannot find module" errors on variations of

import "/packages/mizzao_sharejs-codemirror/.npm/..." import "meteor/mizzao:sharejs-codemirror/.npm/..."

If I load that in a script then the expected "CodeMirror is not defined" error occurs when that file is evaluated.

I feel like I must be missing something key about the module system..

DavidSichau commented 8 years ago

I do not know if there is really a way to import static assets from a module. Maybe we should ask this on the official meteor forum.

This might be related: https://github.com/meteor/meteor/issues/6098

ol-c commented 8 years ago

Being able to load static assets from a module might work. My direct question is: how do I get language modes to work in meteor-sharejs-codemirror? The README.md seems to make that clear for ace. I'd be happy to add documentation for using codemirror modes if I knew the best way to do it.

edemaine commented 8 years ago

I'm also confused about how we're supposed to load the new static .js files. Most of CodeMirror's documentation suggests directly including JavaScript via script tags; not sure what makes sense in Meteor.

A mentioned alternative is to use require("cm/...") to load the module. Should we be depending on NPM module codemirror directly and using require? I couldn't get this to work out (modules load but don't seem to have an effect).

I'm also curious about how to use the .css theme files... I thought these were all going to be included?

DavidSichau commented 8 years ago

Static Assets are not included on default. But they are available to be included either direct or be loaded as a module.

The problem is that often external libraries as codemirror do not support the same module system as meteor. This might result in some difficulties. I have seen them in the ace editor, where ace provided an alternative require function which overwrote the meteor provided require method.

In my opinion there is at the moment no best way. I hope this changes in the future.

edemaine commented 8 years ago

Would you consider just dumping all the CodeMirror code (js and css) into the client when using the CodeMirror package? At least then we could use all the features, until there is a better way...

DavidSichau commented 8 years ago

What do you mean with dumping all CodeMirror code to the client?

Should be all js and css loaded. But how to choose then which css and extensions should be used?

At the moment everything is provided for the user to be included. However the user have to load the js and css which he wants to load.

edemaine commented 8 years ago

The css themes and js modules of CodeMirror are not automatically engaged; each adds a feature to the CodeMirror base, which are then selected by setting options on the CodeMirror object (e.g., editor.setOption 'theme', 'blackboard' triggers CSS with class .cm-s-blackboard). So it's always "safe" to include them (albeit inefficient).

ol-c and I have so far failed to include the js/css as it is included now, though we'd be happy to learn how...

edemaine commented 7 years ago

In my opinion, this issue should be re-opened. I recently figured out how to get access to Meteor-ShareJS's CodeMirror: require("meteor/mizzao:sharejs-codemirror/node_modules/codemirror/lib/codemirror.js"). Critically, note that require("codemirror") does not return the same CodeMirror object (as it's using a local NPM module), which means that any extensions applied to it do not appear in the CodeMirror editors created by Meteor-ShareJS.

Strangely, I cannot require any other CodeMirror modules by a similarly constructed path. But I did finally have luck including CodeMirror modules into Meteor-ShareJS by copying the extension code into my codebase (in a post-install script), and replacing require("codemirror") with the code above. It's not pretty, but I can finally use CodeMirror in my codebase.

The question remains: what is the "right" way to do this? I think a good solution would be require("meteor/mizzao:sharejs-codemirror/node_modules/codemirror/mode/markdown/markdown.js"), but this currently does not work. Any chance this package can be revised so that it works? If we can get this to work, you would no longer need to include the CodeMirror stuff in assets.

DavidSichau commented 7 years ago

The problem is that the npm package of codemirror ships without these dependencies. It only contains the main js file and the css style sheet:

https://github.com/codemirror/CodeMirror/blob/master/package.json#L4-L9

I do not have experience with codemirror, such I am not sure of a way to include the required modes.

But I am open to pull request closing this issue.

edemaine commented 7 years ago

@DavidSichau That's not quite right. package.json's directories specifies "where the bulk of your library is". Only .npmignore and .gitignore cause files to be omitted from the NPM package. At least with meteor npm install codemirror, I get all the auxiliary files too in node_modules. Maybe the Meteor package system is hiding them somehow?

DavidSichau commented 7 years ago

@edemaine For me it would be helpful if you could make an example repo, where you show me the actual problem you want to solve, and how you solve it at the moment. I think I do not understand your concrete problem at the moment.

edemaine commented 7 years ago

@DavidSichau Sure thing: here is a repository, based on a pared-down version of your demo.

This is the line that I want to work, to enable the markdown mode of CodeMirror (for example).

Instead what I've had to do is copy the CodeMirror module and its dependencies, while replacing the opening lines with an import of Meteor-ShareJS's CodeMirror module. I've tried a lot of combinations over the months, but this is the only one I found that worked...

To see that the mode is loading correctly, type **foo** into a document, and it will appear bold.

DavidSichau commented 7 years ago

I now understand your problem but I have not the knowledge how to solve it.

The Problem is that the codemirror files are added as static assets, e.g., they can be accessed via this url:

/packages/mizzao_sharejs-codemirror/.npm/package/node_modules/codemirror/mode/markdown/markdown.js

However with import you cannot import urls:

http://stackoverflow.com/questions/34607252/es6-import-module-from-url

I do not know of a way how one can add a npm package and meteor handles it in a way that all files are loaded into the package. At the moment I believe that meteor only takes the files specified in package.json into account.

mshvern commented 6 years ago

Is there any way to actually apply a mode and theme to CodeMirror editor?