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

Don't commit to supporting the API of our dependencies #7080

Closed
ewinslow opened this Issue Jul 19, 2014 · 21 comments

Comments

Projects
None yet
7 participants
@ewinslow
Member

ewinslow commented Jul 19, 2014

I was just trying to update the markdown library to use composer and realized that it is basically impossible because we provide the existing version using elgg_register_library which is a pretty clear commitment to supporting the full public API of that library. Unfortunately, it's not really feasible to provide a wrapper and leaving it in place is the easiest solution.

I think we should only export exactly the API we want to and not let our dependencies become part of our API. Plugins that depend on a feature should explicitly and directly depend on that feature, not do so "through" Elgg core.

I think this should go for javascript as well, made possible by AMD and explicit dependencies.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jul 27, 2014

Member

I think one of the things causing this problem is that Elgg doesn't have a clean way to resolve duplicate dependencies. So when a shared vendor is needed, the solution is generally to put that vendor in a plugin, then have the 2 dependent plugins depend on it.

I think with the move to composer, this problem can start going away, but not fully until we are 100% in on composer.

We will also eventually need something similar for web dependencies (js, html, css). I have not figured out a way to make composer and bower/volo work together (i.e. when installing a composer lib, also install volo deps). Any ideas here are welcome.

Member

ewinslow commented Jul 27, 2014

I think one of the things causing this problem is that Elgg doesn't have a clean way to resolve duplicate dependencies. So when a shared vendor is needed, the solution is generally to put that vendor in a plugin, then have the 2 dependent plugins depend on it.

I think with the move to composer, this problem can start going away, but not fully until we are 100% in on composer.

We will also eventually need something similar for web dependencies (js, html, css). I have not figured out a way to make composer and bower/volo work together (i.e. when installing a composer lib, also install volo deps). Any ideas here are welcome.

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Nov 27, 2014

Member

Agree on not committing to support our dependencies... should be referenced in docs somewhere..

Member

jdalsem commented Nov 27, 2014

Agree on not committing to support our dependencies... should be referenced in docs somewhere..

@jdalsem jdalsem added the docs label Nov 27, 2014

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Nov 27, 2014

Member

Note that ideally this would include jquery and such as well... Now that we
have and we should make people explicitly declare their own dependency on
jquery. Not something we can do for 1.x, though, I guess.

Member

ewinslow commented Nov 27, 2014

Note that ideally this would include jquery and such as well... Now that we
have and we should make people explicitly declare their own dependency on
jquery. Not something we can do for 1.x, though, I guess.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Nov 27, 2014

Member

Now that we have AMD*

On Thu, Nov 27, 2014, 7:19 AM Evan Winslow evan@elgg.org wrote:

Note that ideally this would include jquery and such as well... Now that
we have and we should make people explicitly declare their own dependency
on jquery. Not something we can do for 1.x, though, I guess.

Member

ewinslow commented Nov 27, 2014

Now that we have AMD*

On Thu, Nov 27, 2014, 7:19 AM Evan Winslow evan@elgg.org wrote:

Note that ideally this would include jquery and such as well... Now that
we have and we should make people explicitly declare their own dependency
on jquery. Not something we can do for 1.x, though, I guess.

@mrclay mrclay modified the milestones: Elgg 1.10.0, Elgg 1.11 Jan 12, 2015

@juho-jaakkola juho-jaakkola removed this from the Elgg 1.11.x milestone Apr 13, 2015

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jun 30, 2015

Member

Do our docs state anywhere that plugin devs should not depend on stuff in composer.json?

cc @jeabakker

Member

mrclay commented Jun 30, 2015

Do our docs state anywhere that plugin devs should not depend on stuff in composer.json?

cc @jeabakker

@ewinslow ewinslow added this to the Elgg 2.0.x milestone Jun 30, 2015

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jun 30, 2015

Member

I don't believe they do.

Member

ewinslow commented Jun 30, 2015

I don't believe they do.

@jeabakker

This comment has been minimized.

Show comment
Hide comment
@jeabakker

jeabakker Jun 30, 2015

Member

@ewinslow said in https://github.com/Elgg/Elgg/pull/8620/files#r33578485

Agree with Steve: let's not encourage folks to have indirect dependencies. Removing one of our requirements should be considered a BC change.

this implies that as a plugin dev i can no longer rely on the fact that jquery is available in Elgg

Member

jeabakker commented Jun 30, 2015

@ewinslow said in https://github.com/Elgg/Elgg/pull/8620/files#r33578485

Agree with Steve: let's not encourage folks to have indirect dependencies. Removing one of our requirements should be considered a BC change.

this implies that as a plugin dev i can no longer rely on the fact that jquery is available in Elgg

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jun 30, 2015

Member

Correct.

Member

ewinslow commented Jun 30, 2015

Correct.

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Jun 30, 2015

Member

As a plugin developer this now requires composer support in all my plugins, even for the basic stuff like jquery, jquery-ui... isn't that asking too much from plugin developers??

Why don't we agree on not committing to support the dependencies, but do commit on keeping them available between major releases?

Member

jdalsem commented Jun 30, 2015

As a plugin developer this now requires composer support in all my plugins, even for the basic stuff like jquery, jquery-ui... isn't that asking too much from plugin developers??

Why don't we agree on not committing to support the dependencies, but do commit on keeping them available between major releases?

@jeabakker

This comment has been minimized.

Show comment
Hide comment
@jeabakker

jeabakker Jun 30, 2015

Member

Why don't we agree on not committing to support the dependencies, but do commit on keeping them available between major releases?

👍

Member

jeabakker commented Jun 30, 2015

Why don't we agree on not committing to support the dependencies, but do commit on keeping them available between major releases?

👍

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jun 30, 2015

Member

For 2.x sure. It would be too breaking. But I also don't want Elgg to grow into a behemoth that can never remove any of its dependencies.

What's so hard about requiring jquery in your plugin via composer? Have you also considered that you might not need jquery at all? Browsers are much better now than they once were, and jquery is quite large.

If requiring plugins to explicitly declare all direct dependencies pushes them to ultimately have fewer dependencies, I'd think that's a good thing, no?

Member

ewinslow commented Jun 30, 2015

For 2.x sure. It would be too breaking. But I also don't want Elgg to grow into a behemoth that can never remove any of its dependencies.

What's so hard about requiring jquery in your plugin via composer? Have you also considered that you might not need jquery at all? Browsers are much better now than they once were, and jquery is quite large.

If requiring plugins to explicitly declare all direct dependencies pushes them to ultimately have fewer dependencies, I'd think that's a good thing, no?

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Jun 30, 2015

Member

Ok... lets discuss this the other way around... if Elgg does not "provide" recommended libraries, you will end up different plugins requiring different (but overlapping) dependencies, like jquery, mootools, prototype. I do not think site owners would like to have all those libraries loaded (because their site will then become a behemoth also).

Why would you really want to be able to remove dependencies in minor releases? Installing them is not the same as loading them... Elgg could maybe never use any functionality of it (so it could be removed), but is keeping the dependency round (until the next major release) for plugin developers

Member

jdalsem commented Jun 30, 2015

Ok... lets discuss this the other way around... if Elgg does not "provide" recommended libraries, you will end up different plugins requiring different (but overlapping) dependencies, like jquery, mootools, prototype. I do not think site owners would like to have all those libraries loaded (because their site will then become a behemoth also).

Why would you really want to be able to remove dependencies in minor releases? Installing them is not the same as loading them... Elgg could maybe never use any functionality of it (so it could be removed), but is keeping the dependency round (until the next major release) for plugin developers

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jun 30, 2015

Contributor

That's the problem I was bringing up in the composer-related ticket. For sites installed and managed without composer, lack of dependency libraries becomes an issue for plugin devs - you never know what to required, define, package etc.

Contributor

hypeJunction commented Jun 30, 2015

That's the problem I was bringing up in the composer-related ticket. For sites installed and managed without composer, lack of dependency libraries becomes an issue for plugin devs - you never know what to required, define, package etc.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jun 30, 2015

Member

Why would you really want to be able to remove dependencies in minor releases?

Install/build performance comes to mind. In any case, waiting until the major release to drop dependencies from composer.json seems like a good compromise for now.

Eventually we should just require composer for managing an Elgg site, I would think. If we want to make it friendly to non-techies then we can build a UI that makes that easier.

Member

ewinslow commented Jun 30, 2015

Why would you really want to be able to remove dependencies in minor releases?

Install/build performance comes to mind. In any case, waiting until the major release to drop dependencies from composer.json seems like a good compromise for now.

Eventually we should just require composer for managing an Elgg site, I would think. If we want to make it friendly to non-techies then we can build a UI that makes that easier.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jul 1, 2015

Member

I think we have a general agreement here not to remove dependencies during a major release cycle.

Member

ewinslow commented Jul 1, 2015

I think we have a general agreement here not to remove dependencies during a major release cycle.

@ewinslow ewinslow closed this Jul 1, 2015

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Jul 1, 2015

Member

Reopening as i would like to see this documented somewhere on learn.elgg.org

Member

jdalsem commented Jul 1, 2015

Reopening as i would like to see this documented somewhere on learn.elgg.org

@jdalsem jdalsem reopened this Jul 1, 2015

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Jul 1, 2015

Contributor

Maybe I don't fully understand the consequences here. Am I required to handle dependencies of plugins by composer, i.e. I MUST add composer support to 2.0 plugins or they might only work with a zipped Elgg installation (with a defined range of dependencies defininitely included) but not on a composer Elgg installation?

Contributor

iionly commented Jul 1, 2015

Maybe I don't fully understand the consequences here. Am I required to handle dependencies of plugins by composer, i.e. I MUST add composer support to 2.0 plugins or they might only work with a zipped Elgg installation (with a defined range of dependencies defininitely included) but not on a composer Elgg installation?

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 1, 2015

Member

@iionly Nothing has changed. If we remove dependencies it'll only be during major releases.

Member

mrclay commented Jul 1, 2015

@iionly Nothing has changed. If we remove dependencies it'll only be during major releases.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 1, 2015

Member

I guess that also includes dependencies only used internally? e.g. Stash.

Member

mrclay commented Jul 1, 2015

I guess that also includes dependencies only used internally? e.g. Stash.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jul 1, 2015

Member

Adding composer requires to your plugins is not strictly required but it's a really really really good idea. It's just a much better way to do development because so much manual effort is removed. It's like a better manifest.xml that actually enforces all the constraints you specify as opposed to just serving as documentation.

Your plugin will work if it depends on something Elgg pulls in, regardless whether the installation is composer or zip (BTW these are really the same thing if you think about it except that we've just executed all the composer commands for you to generate the zip archive).

Member

ewinslow commented Jul 1, 2015

Adding composer requires to your plugins is not strictly required but it's a really really really good idea. It's just a much better way to do development because so much manual effort is removed. It's like a better manifest.xml that actually enforces all the constraints you specify as opposed to just serving as documentation.

Your plugin will work if it depends on something Elgg pulls in, regardless whether the installation is composer or zip (BTW these are really the same thing if you think about it except that we've just executed all the composer commands for you to generate the zip archive).

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jul 1, 2015

Member

Yes, that includes internal only dependencies too. We can always just add new deps and stop using old ones, but we won't remove them for fear of breaking folks...

Member

ewinslow commented Jul 1, 2015

Yes, that includes internal only dependencies too. We can always just add new deps and stop using old ones, but we won't remove them for fear of breaking folks...

ewinslow added a commit to ewinslow/Elgg that referenced this issue Jul 20, 2015

ewinslow added a commit to ewinslow/Elgg that referenced this issue Jul 20, 2015

ewinslow added a commit to ewinslow/Elgg that referenced this issue Jul 20, 2015

ewinslow added a commit to ewinslow/Elgg that referenced this issue Jul 20, 2015

@ewinslow ewinslow closed this in #8746 Jul 21, 2015

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