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

Fails to load in Electron #350

Closed
ColtonProvias opened this issue Sep 23, 2015 · 22 comments

Comments

Projects
None yet
4 participants
@ColtonProvias
Copy link

commented Sep 23, 2015

Due to the way Electron loads JavaScript, AlloyEditor is never bound to window as it sees module first. Thus it fails to load in Electron's renderer process.

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2015

Umm, unfortunately we don't have experience with Electron and we don't know how exactly it loads the JS. Would you like to help us identifying what exactly is the problem?

@ColtonProvias

This comment has been minimized.

Copy link
Author

commented Sep 23, 2015

I'm still rather new to Electron, but they heavily integrate node with it. Thus the JavaScript is loaded as if it is in Node/io.js.

One solution I have found is to modify src/ui/react/src/adapter/main.js and swap the ordering of the conditionals on lines 195-198 so it tests for window first. However, I'm not certain if this will break things for other users, so I'm hesitant about introducing that change as a pull request.

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2015

Go ahead and send us, we will verify it!

@iosonofabio

This comment has been minimized.

Copy link

commented Oct 6, 2015

I have the same problem, created a stub of electron app:

git clone https://github.com/iosonofabio/electroalloy
cd electroalloy
npm install
npm start

and you'll see in Chromium's console that AlloyEditor is not a global (for the reasons @ColtonProvias has hinted at). I thought maybe it's useful for you guys to debug...

@iosonofabio

This comment has been minimized.

Copy link

commented Oct 8, 2015

I had some success with this.

It's mainly about lines 195-198 of src/ui/react/src/adapter/main.js, i.e. lines 937 ff of alloy-editor-all.js and the similar section for React, lines 957 ff of alloy-editor-all.js.

In electron's renderer process (simlar to browserify??):

  • module is an empty object, i.e. it is not undefined
  • window exists

I was able to use Alloy Editor in electron to some degree by forcing window.AlloyEditor = AlloyEditor and AlloyEditor.React = React in those sections, and by loading the code into the HTML by an inline script tag:

<script src="<path-to>/alloy-editor-all.js"></script>
<script>
var React = AlloyEditor.React;
var AlloyEditorComponent = React.createClass({
...
React.render(React.createElement(AlloyEditorComponent, null), document.getElementById('testdiv'));
</script>

(see #217 (comment) for the whole example)

The CSS does not seem to work et caetera, but more or less the functionality is there. So in this usage, one could include an if in those sections for cases in which both module and window are not undefined (i.e. electron).

Now, in principle, electron allows CommonJS (node) -style imports directly in the script tags. This is allowed in an electron index.html:

<script>
var AlloyEditor = require("<path-to>/alloy-editor-all.js");
var React = AlloyEditor.React;
...
</script>

This is obviously a different situation from the inlining above, and more similar to browserify, in which the dependency chain of requires is run down once before window exists. In this case, the exact opposite of the above is supposed to work, i.e. instead of forcing window.AlloyEditor we could force module.exports = AlloyEditor and module.exports.React = React. This has two problems though:

  1. for the case of electron, this populates the global module in case one uses the inline style above, which is probably unwanted behaviour
  2. AlloyEditor.Lang = Lang; fails as shown by other users, because AlloyEditor is not a global for itself (same for OOP, Attribute, etc.)

@ipeychev ipeychev added bug and removed help wanted labels Oct 8, 2015

@ipeychev ipeychev added this to the 0.6 milestone Oct 8, 2015

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2015

I just tried and it was easy to make it working:
electron

The only change I did was to separate the checks for module and window, i.e., instead to check for one or the another, we assume that both can be present, so we export both:

if (typeof module !== 'undefined' && typeof module.exports === 'object') {
    module.exports = AlloyEditor;
}

if (typeof window !== 'undefined') {
    window.AlloyEditor = AlloyEditor;
} else if (typeof self !== 'undefined') {
    self.AlloyEditor = AlloyEditor;
} else if (typeof global !== 'undefined') {
    global.AlloyEditor = AlloyEditor;
} else if (typeof this !== 'undefined') {
    this.AlloyEditor = AlloyEditor;
}

These changes I did in node_modules/alloyeditor/dist/alloy-editor/alloy-editor-all.js since this is the file I loaded in index.html.
I think similar solution was proposed by @ColtonProvias.

No other changes were needed, @iosonofabio. Everything else worked OOTB. Here is the working code.

Can you guys confirm this works for you too? If so, I will commit the code and release v0.6

Thanks,

@iosonofabio

This comment has been minimized.

Copy link

commented Oct 8, 2015

Thanks! That basically works but has issues:

  • by inlining the alloyeditor script, like you do, module.exports in Electron's renderer process is still polluted by AlloyEditor:
<script src="../node_modules/alloyeditor/dist/alloy-editor/alloy-editor-all.js"></script>
<script>
  console.log(module.exports);
</script>
  • requiring AlloyEditor is still not working:
<script>
  var AlloyEditor = require('../node_modules/alloyeditor/dist/alloy-editor/alloy-editor-all.js');
  ... (do things)
</script>

This is bad because it is the most common situation. Typically, people have a React app on electron and they want to plug in AlloyEditor. The app is made of a bunch of CommonJS-style modules that wil get browserified/webpacked. Most modules start with

var React = require('react');

and one would like to add to specific modules a second line:

var AlloyEditor = require('...alloy-editor-noreact.js');

But requiring AlloyEditor does not work...

Uncaught TypeError: Cannot set property 'dir' of undefined

So one could just inline AlloyEditor as a separate script tag before the browserified bundle. But AlloyEditor requires React, which is sitting in the middle of the bundle itself... the typical chicken-egg dependency problem.

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2015

Yes, this is a know issue, but I'm not sure how it can be resolved. Here are more details:

  1. AlloyEditor uses under the hood CKEditr's engine, which is not an CommonJS module and cannot work in non-browser environment.
  2. So far, we were making a difference between NodeJS and browser environment. If NodeJS, we were not running the CKEditor's engine, but we were exporting AlloyEditor as a module. In case of browser, we were running CKEditor's engine and the UI (this combination is actually the AlloyEditor).
  3. If there were React already available on the global namespace, we were reusing it, otherwise, we would use the bundled one.

So, with browserify, there is both module.exports and window. What we should do in this situation? Should we export something to module.exports, to window, or do both? And what to do with CKEditor's engine in this situation?
I'm open to all ideas for solving this problem.

@iosonofabio

This comment has been minimized.

Copy link

commented Oct 8, 2015

Well, CKEditor does not depend on React. So one could inline CKEditor first, followed by a browserify bundle:

<script src="ckeditor.js"></script>
<script src="app.js"></script>

and in the source of app.js we have:

var React = require('react');
var AlloyEditor = require('<path-to>/alloy-editor-core.js');

However, for this to work one should change alloy-editor-core.js slightly. When it checks for react, it only looks for a global variable, but if we are in a node/browserify context (i.e. if module.exports exists) we could fallback onto a require:

if (typeof module !== 'undefined' && typeof module.exports === 'object') {
    React = require('react');
} else if (typeof React === 'undefined') {
    React = AlloyEditor.React;
}

I tried and it almost works, except that:

  • when I load CKEditor separately, no matter what I do with Alloy, I get a strange warning:
[CKEDITOR] editor-plugin-required
plugin: "contextmenu"
requiredBy: "tabletools"
  • I get a missing file for language because it looks for relative paths for lang/alloy-editor/it.js?t=F969 (my machine is curently in Italian)
@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2015

The problem with the language file is more serious, let's concentrate on it for now, later on the plugin.
CKEditor's engine has a special logic for loading the language files. Here it is. AlloyEditor uses the same approach, but with some improvements.

So:

  1. If you load CKEditor separately, you will have to export a global variable, called CKEDITOR_BASEPATH. It should contain the base path, from which CKEditor will load the language files.
  2. In this case you will have to do the same for AlloyEditor (the variable is called ALLOYEDITOR_BASEPATH), or to overwrite the regex, which AlloyEditor uses in order to discover the base path. Please note in CKEditor there is no way to overwrite its regex, this is the improvement I mentioned above.

One more note - when we build the dist file of AlloyEditor, we change the CKEDITOR_BASEPATH with ALLOYEDITOR_BASEPATH, so you have only one variable to overwrite (or one regex). However, when you have CKEditor loaded separately, that's not the case.

@iosonofabio

This comment has been minimized.

Copy link

commented Oct 8, 2015

That works, even with only the ALLOYEDITOR_BASEPATH:

<script>
  //CKEDITOR_BASEPATH = "../deps/ckeditor/";
  ALLOYEDITOR_BASEPATH = "../node_modules/alloyeditor/dist/alloy-editor/";
</script>
<script src="../deps/ckeditor/ckeditor.js"></script>
<script src="app.js"></script>

and the source of app.js:

var React = require('react');
var AlloyEditor = require('./node_modules/alloyeditor/dist/alloy-editor/alloy-editor-core.js');
var editor = AlloyEditor.editable('testdiv');

And I see alloyeditor's UI and I can do things (insert images, tables, etc.).

And about the plugin?

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2015

About the plugin, tell me first please, where did you get ckeditor.js? Is that one which comes from AlloyEditor, or you downloaded it from their site, for example?
Because, such plugin - "tabletools", we don't have. All our plugins have prefix "ae". We also have plugin for dealing with tables, but it is called "ae_tabletools".

@iosonofabio

This comment has been minimized.

Copy link

commented Oct 8, 2015

Indeed I got it from their website, or via bower. I thought that was supported (otherwise what's the point of alloy-editor-no-ck-editor?), am I supposed to use the specific version shipped with alloy or to get the bare version without any plugins from the official CKE website, or do both work?

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2015

No, no, this is exactly the idea - to get the OOTB version from their site and use it.
In Liferay it is done exactly like this, because on one page both CKEditor OOTB and AlloyEditor may exist. This was a killer feature - AlloyEditor then adds just a small piece of JS on top of CKEditor's engine - the UI.
Anyway, I asked what is this CKEditor, because it shouldn't complain of missing plugins. "Tabletools" plugin requires "contextmenu", but it comes with the distribution file. Can you try to load only CKEditor on some "normal" page (not inside Electron)? You should not see any warnings.

@iosonofabio

This comment has been minimized.

Copy link

commented Oct 8, 2015

Results on a normal page (no electron):

  • I can load CKEditor fine
<script src="../deps/ckeditor/ckeditor.js"></script>
  • If I load both CKEditor and alloy-editor-no-ckeditor no complaints
<script src="../deps/ckeditor/ckeditor.js"></script>
<script src="../node_modules/alloyeditor/dist/alloy-editor/alloy-editor-no-ckeditor.js"></script>
  • If I load both and call AlloyEditor.editable, I get the plugin error message:
<script src="../deps/ckeditor/ckeditor.js"></script>
<script src="../node_modules/alloyeditor/dist/alloy-editor/alloy-editor-no-ckeditor.js"></script>
<script>
  var editor = AlloyEditor.editable('testdiv');
</script>

maybe a version conflict? I got CKE 4.5.4 OOTB from the website.

Edit: no plugin error if I download the zero-plugin version of CKE (manually excluding all plugins).
Edit: the basic version of CKE also works, but the standard one (which includes contextmenu and tabletools) does not (well, it generates the error message).
Edit: the standard version minus tabletools (+/- contextmenu) gives a different error, always only when calling AlloyEditor.editable:

Uncaught TypeError: editor.ui.addRichCombo is not a function
@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2015

Got it. The problem is not it the tabletools plugin, it is in their contextmenu plugin. As the name suggests, when you click with the right button, then there is a menu with different items depending on the context.

This plugin comes with basic+ version of CKEditor, which, as you may guess, we immediately remove, see the removePlugins attribute. There is 'contextmenu' in its value. That's why CKEditor complains - "hey, you want me to remove contextmenu plugin, but it is required by tabletools!".
I would recommend you to modify the attribute's value and to add 'tabletools' too, like this:

var editor = AlloyEditor.editable('editable', {
    removePlugins: AlloyEditor.Core.ATTRS.removePlugins += ',tabletools'
});

In this case you will get the same functionality, but with AlloyEditor's UI.

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2015

Hey @iosonofabio,

Let's try to resolve this issue. Finally, you managed to load it to Electron successfully, didn't you? Let's try to summarize the approach you took and see if we can commit the changes you made. In general, what you did was:

  1. You changed main.js like this:
if (typeof module !== 'undefined' && typeof module.exports === 'object') {
    React = require('react');
} else if (typeof React === 'undefined') {
    React = AlloyEditor.React;
}
  1. You separated the loading of AlloyEditor from CKEditor's engine and removed tabletools plugin, like this:
<script src="../deps/ckeditor/ckeditor.js"></script>
<script src="../node_modules/alloyeditor/dist/alloy-editor/alloy-editor-no-ckeditor.js"></script>
<script>
  var editor = AlloyEditor.editable('editable', {
    removePlugins: AlloyEditor.Core.ATTRS.removePlugins += ',tabletools'
  });
</script>

Is the above correct?

@iosonofabio

This comment has been minimized.

Copy link

commented Oct 17, 2015

hi,
sry I'm on a road trip until end of the month with scarce internet, but I'll have time then 100% and will make a pull request OK? I'm all for solving this issue as well...

On October 16, 2015 9:58:55 PM PDT, Iliyan Peychev notifications@github.com wrote:

Hey @iosonofabio,

Let's try to resolve this issue. Finally, you managed to load it to
Electron successfully, didn't you? Let's try to summarize the approach
you took and see if we can commit the changes you made. In general,
what you did was:

  1. You
    changed
    main.js like this:
if (typeof module !== 'undefined' && typeof module.exports ===
'object') {
   React = require('react');
} else if (typeof React === 'undefined') {
   React = AlloyEditor.React;
}
  1. You separated the loading of AlloyEditor from CKEditor's engine and
    removed tabletools plugin, like this:
<script src="../deps/ckeditor/ckeditor.js"></script>
<script
src="../node_modules/alloyeditor/dist/alloy-editor/alloy-editor-no-ckeditor.js"></script>
<script>
 var editor = AlloyEditor.editable('editable', {
  removePlugins: AlloyEditor.Core.ATTRS.removePlugins += ',tabletools'
 });
</script>

Is the above correct?


Reply to this email directly or view it on GitHub:
#350 (comment)

@ipeychev ipeychev modified the milestones: 0.6.1, 0.6 Oct 17, 2015

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2015

Yeah, sure, will postpone this task to 0.6.1 then.

Thanks!

@jepezi

This comment has been minimized.

Copy link

commented Oct 31, 2015

Hi guys. Great project here! Notice that this issue is kinda pending so want to share how I've done it primarily based on #350 (comment). The followings are the steps and the issues I've found along the way.

  1. inline ckeditor.js in html
<script src="ckeditor.js"></script>
<script src="app.js"></script> // <------- bundled by webpack
  1. require/import React and alloyeditor in app.js
var React = require('react');
var AlloyEditor = require('alloyeditor');

At this point, 1st error I got is Uncaught ReferenceError: AlloyEditor is not defined. So I modified alloy-editor-core.js a bit here.

  1. Before L220, I've added this line.
var AlloyEditor = module.exports; // <------------ add this
var React = (function() {
...
  1. Add React check according to @iosonofabio
if (typeof module !== 'undefined' && typeof module.exports === 'object') {
    React = require('react');
} else if (typeof React === 'undefined') {
    React = AlloyEditor.React;
}
  1. Add removePlugins config
this.editor = AlloyEditor.editable('editable', {
  removePlugins: AlloyEditor.Core.ATTRS.removePlugins += ',tabletools'
});

After these steps I got it working now.

@iosonofabio

This comment has been minimized.

Copy link

commented Oct 31, 2015

Hi @jepezi, thanks for the heads up. I am planning to draft a PR today or tomorrow. Didn't you have to change ALLOYEDITOR_BASEPATH like I did above?

ipeychev added a commit that referenced this issue Nov 7, 2015

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Dec 25, 2015

Here is a repository with an example of React + Webpack, might help in this case too.

Thanks,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.