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

Can't use with requirejs #240

Closed
jamwise opened this issue Jun 10, 2015 · 27 comments
Closed

Can't use with requirejs #240

jamwise opened this issue Jun 10, 2015 · 27 comments

Comments

@jamwise
Copy link

jamwise commented Jun 10, 2015

I've been trying to get this to work with requirejs and with shims even trying to manually change the files to add in the module wrapper isn't getting anywhere. Is there a simple way to use this with requirejs?

@ipeychev
Copy link
Contributor

Hey @jamalneufeld,

Thanks for your report! We don't have OOTB way to load it from RequireJS, but we can think and create one.

Could you please explain how would you expect that to work?

Thanks,

@jamwise
Copy link
Author

jamwise commented Jun 10, 2015

Well I've been trying to make a fork that would work but I'm not familiar enough with your codebase and am messing things up. Essentially I'm looking for alloy-editor to have an export that's an AMD module link here. This would mean it doesn't exist in the global (window) namespace and that the dependencies it needs can be pulled in through require as well.

The other part of this is that I want to use alloy-editor within a react app. The fact that it's partly built in react is what interested me. But because AlloyEditor.editable(id) only takes an id makes that hard. If it could also accept a DOM object, like what you get when you run getElementById(), then react can manage connecting it to the rendered elements and avoid some issues.

Let me know your thoughts, and if I can help in any way.

@jamwise
Copy link
Author

jamwise commented Jun 10, 2015

also, if I want to include the editor the way you suggest, it clashes with requirejs and has strange errors:
Uncaught ReferenceError: React is not defined alloy-editor-all.js:24495
even thought I'm using the version that includes all

@ipeychev
Copy link
Contributor

Maybe what should be added is another template similar to the default wrapping template:
https://github.com/liferay/alloy-editor/blob/ab74529b50d9858fbc5eeb6ae1f3f80ff9baeec2/src/ui/react/template/alloy-editor.template

In the new one we can wrap we editor in an AMD module instead to expose it to the global namespace.

also, if I want to include the editor the way you suggest, it clashes with requirejs and has strange errors:
Uncaught ReferenceError: React is not defined alloy-editor-all.js:24495
even thought I'm using the version that includes all

Okay, will check this.

The fact that it's partly built in react is what interested me. But because AlloyEditor.editable(id) only takes an id makes that hard. If it could also accept a DOM object, like what you get when you run getElementById(), then react can manage connecting it to the rendered elements and avoid some issues.

It should, the function "editable" accepts actually two parameters: the ID or the DOM element and configuration object. It is just a bug that it does not accept DOM element anymore which we are going to fix very, very soon.

@jamwise
Copy link
Author

jamwise commented Jun 10, 2015

I'm not sure if that would be enough, as there are quite a few references to window in the core. If you have any thoughts about why I can't even use the editor in the global namespace that would help out a lot for the time being.

@ipeychev
Copy link
Contributor

I'm not sure if that would be enough, as there are quite a few references to window in the core.

Yeah, of course we will update these.

If you have any thoughts about why I can't even use the editor in the global namespace that would help out a lot for the time being.

I am not sure what you mean. Do you mean that you can't use the editor as stated here? If so, can you provide the steps for reproducing the issue?

@jamwise
Copy link
Author

jamwise commented Jun 10, 2015

Got it, your embedded react (or even if it's included elsewhere in the page) has this check: if(typeof define==="function"&&define.amd){define([],f)} and since I have requirejs on the page, define is a function which contains amd. Since this is true, react isn't injecting itself into window.

@ipeychev
Copy link
Contributor

Alright.

Meanwhile, the bug in which editable didn't accept DOM element has been fixed.

@jamwise
Copy link
Author

jamwise commented Jun 13, 2015

So requirejs usually is able to shim a file so it works without much issue. I've been able to do this for a lot of other libraries, but until just now I didn't realize why it wasn't working for alloy. And the reason is the extra settings and language files. If those are able to be minified into the single source, and have it not attach to window, then it would work like a charm.

@ipeychev
Copy link
Contributor

We could bundle them to a single file. However, we currently have about 66 language files and only one of them would be used by the editor on runtime. This will mean we will waste the rest 65 files and they are not so small - the size of the editor's file would increase with about 62Kb. That is the only reason they are not bundled, but loaded separately.

@jamwise
Copy link
Author

jamwise commented Jun 13, 2015

Makes sense. And in fact I think this isn't causing the issue. I got alloy working with require last night but none of the icons were showing up and it was working strangely. I though it was require until I tried the latest demo index.html inside the dist folder and saw the icons were missing there too. Am I doing something wrong in the build process? Or was a bug introduced with the last update?

@ipeychev
Copy link
Contributor

I just checked. The reason is in gulp-iconfont - it behaves differently or it is buggy. Downgrade it please to 1.0.1 in your package.json as it was before until we find the reason and commit a fix.

@ipeychev
Copy link
Contributor

Fixed in v0.3.2.

@djsliders
Copy link

Hi.
Did someone manage to add this great module with requireJs? Would it be possible to have a working example?

Thank you very much for your work.

@jamwise
Copy link
Author

jamwise commented Jun 14, 2015

Hi @djsliders, I was able to get it to work with a shim. But the issue I was running into was having react built-in. React was checking first if the AMD module system is present, and if it is, it wasn't injecting itself into window, and alloy editor requires react to be on window. So In order to get it working use the no-react version and manually add react to the window like this:

require.config({
    paths: {
        react: 'bower/react/react-with-addons',
        alloyEditor: 'bower/alloyeditor/alloy-editor-no-react'
    },
    shim: {
        'alloyEditor': {
            exports: 'AlloyEditor'
        }
    }
});
require(['react'], function(React) {
    window.React = React;
    return React;
});

Then you can require AlloyEditor in any module without issue. This isn't ideal since it forces React to be on window, and because React barfs when there are several instances of it, if you're using React elsewhere you'll have to modify them to use the global React. But it'll work at least.

@djsliders
Copy link

Thanks. The thing is that I have an isomorphic application. Meaning that I have Node.js on the server side and window is not defined :/
I think this module is great so I really want to make it work :)

@jamwise
Copy link
Author

jamwise commented Jun 15, 2015

Yup, I love isomorphic react :) You'll have to "hide" alloy/ckeditor when on the server. Since it just uses a dom element with contenteditable, you can load that div with dangerouslySetInnerHTML without instantiating alloy. They instantiating it after it loads into the browser

I'd be interested to see how you have alloy set up in react so far. You should be able to get it to work isomorphically with some if statements. You can use this to determine where you are:

var isNode = typeof module !== 'undefined' && module.exports
, React = isNode ? require('react/addons') : window.React

@ipeychev
Copy link
Contributor

Hey @jamalneufeld,

Thanks for your work on making AlloyEditor to load with RequireJS, this was spectacular job! Would it be possible for you to write a guide for the site?
We think this will be very useful for the other people.

Thanks,

@jamwise
Copy link
Author

jamwise commented Jun 16, 2015

Hi @ipeychev, I would be glad to, but I feel like my current method is still a bit rough. I'm planning on forking and implementing some changes I've made which will make it easier to implement and more in line with modular js. If you guys are already working on that I'll be happy to collaborate, and either way once I'm done you'll get a pull request from me. At that point I'll gladly make a guide ;)

@ipeychev
Copy link
Contributor

@jamalneufeld Even better, we would be happy to get a PR and a guide, thanks a lot!

@kkotwal94
Copy link

Would it be possible to use this with es6 and webpack as well?

@ipeychev
Copy link
Contributor

For some package managers and loaders we can add support, like we did for NPM and Bower. However, for others, which we don't use, neither know well, we would be happy to receive a PR or a guide.

@kkotwal94
Copy link

Does PR stand for project request?

@ipeychev
Copy link
Contributor

No, it stands for pull request.

@kkotwal94
Copy link

okay

@ipeychev
Copy link
Contributor

ipeychev commented Dec 6, 2015

This should work now, please reopen if there are still some issues.

@ipeychev ipeychev closed this as completed Dec 6, 2015
@magestican
Copy link

This problem is going on again, whenever I try to add alloyeditor through require I get errors like :

(index):1 Uncaught SyntaxError: Unexpected token <
alloy-editor-no-react.js?ec37:14330
And :
Uncaught TypeError: Cannot read property 'add' of undefined(…)

I was getting this error trying to import CKEDITOR using require and it turned out there were many files missing from the main ckeditor.js file which had to be bundled.

Any help on this? I am trying to use this editor with a react application, I followed the tutorial but it had these issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants