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

Possible BC break in new widgets AMD module #9523

Closed
mrclay opened this Issue Mar 20, 2016 · 8 comments

Comments

Projects
None yet
2 participants
@mrclay
Member

mrclay commented Mar 20, 2016

See ColdTrick/widget_manager#76

@hypeJunction details would be great here.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 20, 2016

Contributor

I am not sure what is happening exactly. If I wrap ColdTrick's JS to require elgg/widgets and call init() it works, otherwise init() is never called due to error (or would have been called too late).

Contributor

hypeJunction commented Mar 20, 2016

I am not sure what is happening exactly. If I wrap ColdTrick's JS to require elgg/widgets and call init() it works, otherwise init() is never called due to error (or would have been called too late).

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 21, 2016

Member

There are a few issues. In 2.1, init, system fires later, in the elgg/init module. I don't think this is a bug, but if a lot of plugins assume Elgg stuff is done by $ domready, this may cause problems.

Also, widgets.init() is no longer tied to init, system. That's probably not OK. We probably need elgg/init to depend on any "always required" core modules, at least elgg/widgets.

Lastly, widget_manager's site.js doesn't block on anything but the elgg module, so it's basically working on the luck of init, system firing really early.

Member

mrclay commented Mar 21, 2016

There are a few issues. In 2.1, init, system fires later, in the elgg/init module. I don't think this is a bug, but if a lot of plugins assume Elgg stuff is done by $ domready, this may cause problems.

Also, widgets.init() is no longer tied to init, system. That's probably not OK. We probably need elgg/init to depend on any "always required" core modules, at least elgg/widgets.

Lastly, widget_manager's site.js doesn't block on anything but the elgg module, so it's basically working on the luck of init, system firing really early.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Mar 21, 2016

fix(js): again calls widgets.init in the init, system event
In 2.0 the `widgets.init` was called on `init, system`. In 2.1 it was called
whenever the `elgg/widgets` module was loaded, but this introduces the problem
that plugins that worked on 2.0 no longer have a reliable blocking point to
make sure `widgets.init` has been called.

Due to BC, we cannot require plugins depend on the new `elgg/widgets` module,
but we can at least provide assurance that `widgets.init` will have been
called after `init, system`.

Hence, we now call it in `init, system` as before, and we delay `elgg/init`
until `elgg/widgets` has loaded.

Fixes #9523
@mrclay

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 22, 2016

Contributor

The issue is bigger here.
In 2.0, dependency on elgg was sufficient for the init,system to fire. Now you have to depend on elgg/ready.

Contributor

hypeJunction commented Mar 22, 2016

The issue is bigger here.
In 2.0, dependency on elgg was sufficient for the init,system to fire. Now you have to depend on elgg/ready.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 22, 2016

Contributor

This is 2.0 code:

define('elgg', ['jquery', 'languages/' + elgg.get_language()], function($, translations) {
    elgg.add_translation(elgg.get_language(), translations);
    $(function() {
        elgg.trigger_hook('init', 'system');
        elgg.trigger_hook('ready', 'system');
    });
    return elgg;
});

In 2.0, modules that depend on elgg were sure that all the core and plugin code registered for init was bootstrapped. It's not the case anymore, that's why widget_manager fails, and I am sure we will see more failing JS code.

Contributor

hypeJunction commented Mar 22, 2016

This is 2.0 code:

define('elgg', ['jquery', 'languages/' + elgg.get_language()], function($, translations) {
    elgg.add_translation(elgg.get_language(), translations);
    $(function() {
        elgg.trigger_hook('init', 'system');
        elgg.trigger_hook('ready', 'system');
    });
    return elgg;
});

In 2.0, modules that depend on elgg were sure that all the core and plugin code registered for init was bootstrapped. It's not the case anymore, that's why widget_manager fails, and I am sure we will see more failing JS code.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 22, 2016

Contributor

In case of widget_manager, I think it's up to them to fix. The plugin extends elgg.js and does not use an async module. Even though the behaviour did change between 2.0 and 2.1 with regards to when UI libraries are bootstrapped, I think we should discourage plugins from extending elgg.js, especially without hook handler registration. @jdalsem, thoughts?

Contributor

hypeJunction commented Mar 22, 2016

In case of widget_manager, I think it's up to them to fix. The plugin extends elgg.js and does not use an async module. Even though the behaviour did change between 2.0 and 2.1 with regards to when UI libraries are bootstrapped, I think we should discourage plugins from extending elgg.js, especially without hook handler registration. @jdalsem, thoughts?

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 22, 2016

Contributor

@mrclay Sorry, I should have just paid attention to your message :) Just repeated everything you have said. I don't think there is anything to fix here. Let's document that extending elgg.js is not ok, and that you should depend on init/ready, if you do. It's a trivial fix for plugins to go from elgg to elgg/ready or bind to ready event.

Contributor

hypeJunction commented Mar 22, 2016

@mrclay Sorry, I should have just paid attention to your message :) Just repeated everything you have said. I don't think there is anything to fix here. Let's document that extending elgg.js is not ok, and that you should depend on init/ready, if you do. It's a trivial fix for plugins to go from elgg to elgg/ready or bind to ready event.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 22, 2016

Member

I've actually come around to accepting extending elgg.js as long as async require is used. I think the big overhaul I was planning to make elgg truly async, just isn't worth the trouble for us or for plugin devs.

Member

mrclay commented Mar 22, 2016

I've actually come around to accepting extending elgg.js as long as async require is used. I think the big overhaul I was planning to make elgg truly async, just isn't worth the trouble for us or for plugin devs.

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