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

npm package is broken. [EDIT: Does not work with browserify] #338

Closed
andreasheim opened this Issue Sep 5, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@andreasheim

andreasheim commented Sep 5, 2015

My steps:

$ npm install alloyeditor --save
alloyeditor@0.5.2 node_modules/alloyeditor

Then in my app (using gulp and browserify):

AlloyEditor = require 'alloyeditor'

When opening my app in the browser, I get:
screen shot 2015-09-04 at 6 25 33 pm

If I embed the full version:

AlloyEditor = require 'alloyeditor/dist/alloy-editor/alloy-editor-all'

I get the same exception - so it's not the dependencies.
screen shot 2015-09-04 at 6 26 28 pm

Looking at the source, I see this:

(function() {
    'use strict';

(function () {
    'use strict';

    /**
     * AlloyEditor static object.
     *
     * @class AlloyEditor
     * @type {Object}
     */
    var AlloyEditor = {

// ...

    }
})();

    var React = (function() {
        return (0, eval)('this').React;
    }());

    if (typeof React === 'undefined') {
        React = AlloyEditor.React;
    }

So that nested 2nd closure is closed before doing the React check. At which point AlloyEditor has not been declared since this code runs outside the nested closure.

I can get past that by using

var React = require('react');

and then further down

require('./ckeditor');

Yet then it still fails here:

screen shot 2015-09-04 at 6 43 11 pm
screen shot 2015-09-04 at 6 42 32 pm

@andreasheim

This comment has been minimized.

andreasheim commented Sep 5, 2015

CKEditor is embedded in the core file, but I suppose not until further down. So the ordering of how things are stitched together doesn't work?

@ipeychev

This comment has been minimized.

Contributor

ipeychev commented Sep 6, 2015

I wouldn't say the npm package is "broken".
AlloyEditor's dist files are just not meant to be used with browserify for at least two reasons (explained below). Here is an example how the npm package was meant to be used. We don't think using browserify is a good way to load AlloyEditor, but we don't mind to make it working with it. However, for now we would prefer to receive a pull request from the community to support this instead to give it priority in our roadmap.

In general, to bundle AlloyEditor's files inside your application does not make much sense and we don't recommend you to do it for at least these reasons:
1.You will change the files of your application much often than AlloyEditor. If you load AlloyEditor separately from the other files of your application, the browser will most likely cache it. Then, when you change the files of your application, they will be downloaded again, but not AlloyEditor's files.
2. There is another issue with bundling AlloyEditor inside your application. When you do this, you will most likely change the name of the file and this will prevent AlloyEditor to resolve and load the corresponding language file. The algorithm for loading it is similar to those of CKEditor. Read here for more information why is this needed. Keep in mind AlloyEditor goes one step further and instead to set ALLOYEDITOR_BASEPATH, you can change the regex, which looks for AlloyEditor's script file.

Now about the issue:
I just tried to bundle AlloyEditor using browserify and it seems in this case it adds module.exports. The issue is that in this case the current build files assume this is NodeJS environment and not a browser. That's why it tries to expose AlloyEditor.Lang and the exception you posted occurs.

@dantman

This comment has been minimized.

Contributor

dantman commented Sep 6, 2015

1.You will change the files of your application much often than AlloyEditor. If you load AlloyEditor separately from the other files of your application, the browser will most likely cache it. Then, when you change the files of your application, they will be downloaded again, but not AlloyEditor's files.

That is what --require/--external and factor-bundle are for.

  1. There is another issue with bundling AlloyEditor inside your application. When you do this, you will most likely change the name of the file and this will prevent AlloyEditor to resolve and load the corresponding language file. The algorithm for loading it is similar to those of CKEditor. Read here for more information why is this needed. Keep in mind AlloyEditor goes one step further and instead to set

There's also the option of making require('alloy-editor') return a factory function instead of AlloyEditor itself. That function could accept a base dir for resources or a method that can be called to load resources.

For an example see how jQuery works on the server.

@ipeychev

This comment has been minimized.

Contributor

ipeychev commented Sep 7, 2015

1.You will change the files of your application much often than AlloyEditor. If you load AlloyEditor separately from the other files of your application, the browser will most likely cache it. Then, when you change the files of your application, they will be downloaded again, but not AlloyEditor's files.

That is what --require/--external and factor-bundle are for.

Sure, thanks for the heads up!
Mine was a warning, previously I've seen exactly that - people were trying to bundle AlloyEditor directly inside their applications.

There is another issue with bundling AlloyEditor inside your application. When you do this, you will most likely change the name of the file and this will prevent AlloyEditor to resolve and load the corresponding language file. The algorithm for loading it is similar to those of CKEditor. Read here for more information why is this needed. Keep in mind AlloyEditor goes one step further and instead to set

There's also the option of making require('alloy-editor') return a factory function instead of AlloyEditor itself. That function could accept a base dir for resources or a method that can be called to load resources.
For an example see how jQuery works on the server.

Sounds good for me. Would you guys like to contribute code which fixes this issue?

@andreasheim

This comment has been minimized.

andreasheim commented Sep 7, 2015

So I see that module.exports is present. The problem I see is that the rest of the code assumes that AlloyEditor is available globally. That only works when window is present, and window.AlloyEditor is set (the default behavior).

Yes, I'm aware that the library (in particular the ckeditor dependency) is massive. I didn't go in detail there, it will/would go into a vendor/library bundle, not the core app.js.

In our environment, all files are hashed. So libraries need to be static and not load other files dynamically merely based on a base folder. I had hoped that since I discovered alloy on npm it would include a version of ckeditor that would work with these requirements.

I'd be willing to contribute to fix this issue, but I'm not sure if I'll have the time in the short term :(

@ipeychev

This comment has been minimized.

Contributor

ipeychev commented Sep 8, 2015

Well, if you know the language of the browser in advance, or if you don't have to support multiple languages, then you can provide the needed language file (for example English one) explicitly. As far as I remember the config file too... so in general the editor won't load any additional files dynamically.

About the issue, here is what the editor does - in browser environment, it exposes all the needed API. In NodeJS, it exports an object, so you are able to render the page on the server and to write some unit tests. However, with browserify in the moment of initialization both are available - module.exports and window. This is what is not expected currently.

@ipeychev ipeychev changed the title from npm package is broken. to npm package is broken. [EDIT: Does not work with browserify] Sep 9, 2015

@ipeychev ipeychev added the bug label Sep 9, 2015

@ipeychev ipeychev added this to the 0.7.0 milestone Nov 7, 2015

@ipeychev ipeychev closed this in a8d0c73 Nov 7, 2015

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

@ipeychev

This comment has been minimized.

Contributor

ipeychev commented Dec 25, 2015

Here is a repository with an example of a React component, server-side rendering and creating an instance in the browser.

Hope that helps!

Thanks,

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