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

Adding plugin version fetching proposal #30

Merged
merged 4 commits into from Mar 18, 2016

Conversation

riknoll
Copy link
Contributor

@riknoll riknoll commented Dec 3, 2015

This is a counterproposal to pinning core plugin versions in cordova-lib aimed at adding support for third party plugins

@stevengill
Copy link
Contributor

Hey Richard,

Good looking proposal!

If implemented well and adopted by our plugin developers, I definitely see the value this provides.

Things I like:

  1. using major versions of plugins to specify engine dependencies
  2. moving engine tag info to package.json and using npm info to gather it
  3. autogen of cordovaDependencies
  4. cordovaDependencies structure you have laid out.
  5. psedocode logic looks good

Some cons

  1. I think this will require a good amount of work and time to implement in a way that is stable and covers all use cases.
  2. maintaining ranges can easily become annoying and lag behind. It really requires us to be on top of updating the engine tags often(which we really should be doing more).

Few questions:

  1. for the cordova dependency, is the plan to support a major version (^5.0.0)? Would it be a range (>3.0.0 <5)?
  2. Would we have to do a plugins release for every version of the tools to update the range? Every major version?
  3. Would we go back and update the range for older plugin versions?
  4. How would autogen of cordovaDependencies handle ranges? Just copy what engine already has I assume.
  5. Would we autogen cordovaDependencies for previous releases as well? Based on tags or versions on npm I would guess.

Other thoughts

  • Documenting the hell out of this should be in the proposal so plugin authors know how to add it
  • Instead of generating cordovaDependencies from engine tag in plugin.xml, why don't we just move the engine tag logic over to package.json completely. Your proposal seems robust enough to just replace it. We would have to update the tools to read from package.json first and only check plugin.xml if cordovaDependencies doesn't exist.

I think this could provide many benefits for cordova and the ecosystem. If we are still planning on trying to get cordova 6 out asap (next week), I don't think this is going to make the cut (especially considering testing time and updating all of the plugins). But it would be a really great goal for cordova 7 ;).

This could easily work with the other pinning proposal as well. In your pseudocode where you grab latest if cordovaDependencies doesn't exist, it could grab the pinned version instead of latest ensuring the version of cordova and its pinned platforms were tested with that version of the plugin. Using it as a last resort.

@stevengill
Copy link
Contributor

Chatted with @purplecabbage a bit.

For the usecase where the latest release of the plugin specifies cordova: ^5.0.0 and we released cordova 6, instead of the plugin failing due to not meeting engine requirements, maybe we can do what node does. Something like expected cordova 5.0.0, will install anyways. That way, we can still encourage plugin developers to use a top range, but don't fail.

I'd hate to see a plugin use a maximum top range and then we release a new version of cordova and the plugin fails to install due to engine failure even though the plugin would still work.

@riknoll
Copy link
Contributor Author

riknoll commented Dec 4, 2015

Thanks for the review Steve! Here are some responses to your questions:

  1. I think we can be pretty flexible here. Ranges and major versions should both be supported
  2. I believe that we will probably have to do a plugin release sometimes for major version bumps on the tools. Non-major version changes should be fine so long as they are truly non-breaking. I agree with you and @purplecabbage regarding the upper ranges for pinned major versions in the latest version of a plugin. However, explicit upper bounds (e.g. cordova-android: <5.0.0) and all lower bounds should always be respected. The user never wants to install a plugin with that engine entry if it's just going to fail with permission errors. We just need to make sure we only use explicit ranges like that when we are sure about upcoming incompatibilities. The user can always override it by specifying latest when they install the plugin.
  3. Going forward, I think we should at least update the previous version's engine info when we do a plugin release. Using the cordova-android 5.0.0 example again, when we release a 5.0.0 compatible version of a plugin, the previous version should get a cordova-android: <5.0.0 engine entry if it didn't have one already.
  4. Yep! I want to use the existing plugin.xml information as much as possible.
  5. Ideally yes, but getting all that engine information up to date sounds pretty arduous. The intention of the autogen was to use the existing engine information in old versions of plugin.xml to get a good start on this task. I think we'll probably be okay focusing on making sure we have good engine information in latest and maybe one release back for now. Autogen should probably be based on npm versions.

I agree that this requires comprehensive documentation before implementation, I just wanted to get the idea up here for discussion first. Moving the engine tag to plugin.xml makes sense to me as well (no need to have it in two places!). This is definitely compatible with plugins pinned in cordova-lib. We just need to make sure that we don't leave out third party plugins in whatever we do.

@dblotsky
Copy link
Contributor

dblotsky commented Dec 5, 2015

Just adding my two cents here.

@stevengill: automatically updating to major versions is not very stable practice, but if it's required just using a range or >= would work.

Also @stevengill, I'm personally not a huge fan of putting dependencies in cordova-lib (I missed expressing this when the earlier proposal came out, sorry!) because the dependencies are of plugins on platforms, not the other way around. It feels like an antipattern to encode dependents in the thing they depend on, and as a result it also misses third-party plugins. About both of these things (tools pinning plugins and plugins pinning platforms) coexisting: I think it would be best to go with only one of them since maintaining both is more error-prone.

@stevengill
Copy link
Contributor

  1. Good

  2. The explicit upper bound one is going to be tricky. Over the course of cordova's history, we have had lots of plugins developed that then get abandoned due to the developer moving on. These plugins sometimes will continue to work in the state that they were left. What happens if a plugin has a set upper bound and then gets abandoned?

    We are down the path now that any major platform update = major tools update. If cordova-windows goes to 5.0.0 in a couple of months, cordova-cli will have to jump to 7. Why would a plugin that still supports cordova-android@5 and cordova-ios@4 start failing due to the tools being bumped up. The plugin might not even support cordova-windows but this change could cause it to be incompatible. It is easy enough for us to manage our own plugins and make sure they are up to date by doing a new release. But 3rd party plugin developers might not be willing or able to keep up (abandoned).

    Being explicit about using latest isn't necessarily a workaround because the engine tag gets checked at install time currently by our tools. It would see that the version of cordova is incompatible with the plugin.

  3. Sounds good. Just requires us to do another vote + release for the past version but not a huge deal.

  4. Sounds good.

  5. Agree with latest and one release back.

Glad you agree with moving engine tag from plugin.xml to package.json!

@dblotsky
Copy link
Contributor

dblotsky commented Dec 5, 2015

@stevengill: I posted a comment between Richard's last comment and your last comment. Any thoughts?

@dblotsky
Copy link
Contributor

dblotsky commented Dec 8, 2015

@stevengill Ping.

@riknoll
Copy link
Contributor Author

riknoll commented Dec 8, 2015

@stevengill after talking with @dblotsky and others, I have to agree that pinning plugins in two places (cordova-lib and the plugins themselves) is just creating more maintenance and opportunities for things to go out of sync. I'd rather these dependencies live in one place. Should we hold on releasing that feature in Cordova 6.0?

@stevengill
Copy link
Contributor

The whole point of pinning plugins was to add some stability to the cordova ecosystem. This was all discussed at the f2f fairly in-depth.

The main point of pinning plugins is to say to our users "Look we tested this version of the CLI with these platforms and these plugins. We can guarantee that they work together." It is that simple.

Yes, pinning plugins does not provide a solution for 3rd party plugins. It was only ever meant to add stability for our core plugins. We could theoretically pin popular third party plugins as well, but I'd prefer not to go down this route.

Once again, I really like the above proposal by @riknoll. I think we have some details to figure out still but it looks very promising. It is however going to be way more work and I bet more error prone (core and third party plugins not being updated fast enough)(we do a plugins update and now devs in teams using cordova have could have different versions of plugins in their projects depending on when they restored/added plugins). I don't think you have fully thought out all of the use cases these changes can affect.

We only update the pinned plugins when we do a plugins release. When it is time to do a tools release, we run mobile spec with released platforms and released plugins. At the time of release, all of these should be pinned. I don't think it adds much complexity or opportunities to go out of sync. In fact, I'd argue it is more stable as it is exactly what we tested when releasing.

At release time for tools, a plugin must support the versions of platforms pinned to the tools. If not, how is that plugin ever going to work. Our tools don't support installing two different versions of a plugin in one cordova project. If you have a cordova project with ios@3.9.2 and android@4.0.0, then the plugins you install need to support both. If file@3 supports ios@3.9.2 but doesn't support android@4.0.0, but file@4 supports android@4.0.0 but not ios@3.9.2, we can't release that combination! So at release time, it is our opportunity to confirm that what is released works well together. If not, we need to either update the pinned platform, or update the plugin. (The above proposal doesn't seem to handle this case either.)

@dblotsky I wouldn't look at pinned platforms and plugins as dependencies. It is incredibly easy to change what version of platforms and plugins to add. cordova plugin add cordova-plugin-device@VERSION. They are just pinned to state what we tested when released, therefore what we guarantee will work well together.

Our original versioning and releasing strategy which we have been following for 2+ years made it clear that major jump in platforms != major jump in tools. This is what we have been doing for quite a while now. I was one of the main supporters that CLI version not be tied to platform versions. That CLI be able to work with any version of platforms if we can help it. We have had many versions of the cli that worked with past and future versions of platforms and plugins. We have done a pretty decent job of making changes backwards compatible. Issue is that we never had the man/machine power to go back and test. I can't say if cordova@3.1 works with android@4. To properly use engine tag would require us to test older versions with newer versions and vice versa.

This has caused SOOOOO MUCH confusion for cordova developers. We are at a point where developers are afraid to update. Developer teams end up getting different versions of different things when they want to work on the same project. This is leading them to check in plugins and platforms into github. --save solves this but I don't think it is common practice based on how many people I have had to teach about it. It is probably worth auto --save but that is a different discussion.

This is what has finally led to the current versioning strategy. Which is major change in platform == major change in tools. Platforms are essentially tied to tools now. This doesn't mean tools don't support other versions of platforms. This is ugly because it isn't proper semver. But we tried proper semver for the last 2 years and it isn't working for us. We are essentially still doing semver except for the exception when a platform does a major change. When our users think of the version of cordova, they think of the version of cordova-cli. They don't think about the version of cordova-android, cordova-ios, etc. They see it as one. We release it as one. The version of cordova has to reflect runtime changes for platforms for our users benefit. Most times, a major runtime change includes changes to cordova-lib that it requires to function properly.

It was agreed at the F2F that cordova needs to focus on stability. This is the first major step towards that. Future releases of tools will no longer grab the latest by default. They will only grab what has been tested with them and the platforms they have pinned.

We can revisit removing pinned dependencies once this proposal is implemented, tested, plugins updated, 3rd party plugin developers notified and updated, and documentation is ready. Not to mention the ironing it out it still needs in terms of use-cases for teams. I like the potential of the proposal, but am very worried about how it will do in real life projects and how much of a maintenance burden it will add for cordova committers. Cordova community has definitely had issues in the past with new features added in and then abandoned once committing companies move on.

As you can see, we have a lot of work to do. This is not something that should hold back cordova-6.0.0.

Sorry for diving into the platform versioning too. I know @dblotsky posted to the list about dependency hell, I just thought it made more sense to respond to those queries here as they are related.

@stevengill
Copy link
Contributor

It is very important for us to keep progressing and releasing. Which is why we need to move on cordova 6.0.0. Users have been clamoring for marshmallow and wkwebview support for too long now.

@riknoll
Copy link
Contributor Author

riknoll commented Dec 9, 2015

@stevengill I wasn't suggesting blocking the 6.0.0 release, just plugin pinning. I agree that this is a complicated feature and that the proposal above is not fleshed out to the point of implementation (and certainly not something that would be done for 6.0.0). However, I'm willing to concede that a core-only solution is useful as a temporary measure before a more complete one is implemented (whether that final solution be derived from this proposal or some other idea). I agree that stability is one of the key issues in the Cordova ecosystem, I just want to ensure that we are moving in a direction where we will eventually be able to extend stability to all parts of the ecosystem and not just core. Sorry if I was unclear in my earlier posts!

@stevengill
Copy link
Contributor

If this proposal is implemented well, it would definitely remove the need for pinning plugins. The benefit to the community and ecosystem would be huge.

By having pinning in the next release, we are essentially future proofing cordova. My hope is someone can download cordova@6 a year from now and it still be able to work.

We could even one day get to a point where we use this type of proposal on platforms themselves. I just want to avoid issues for our developers along the way.

@dblotsky
Copy link
Contributor

dblotsky commented Dec 9, 2015

I agree with @riknoll that this is a decent temporary measure, but not a long-term solution. I'm also willing to support installing pinned plugins in 6.0.0 as an interim measure, but to then make a proper solution for the versioning problem take higher priority over future feature work.

Regarding the broader issue of versioning and the long-term solution post-6.0.0, let's continue to discuss on the mailing list so that others can participate.

@riknoll
Copy link
Contributor Author

riknoll commented Jan 14, 2016

This has been updated!

I've tried to clarify some things and update the semantics based on our discussion and some offline conversations with @dblotsky and @axemclion. I think this covers most of what we talked about:

  • Fetch pinned plugins instead of latest if possible
  • Version constraints are flexible (npm semver)
  • Adding never fails based on constraints (fall back to latest/pinned versions with warnings)

I also wanted to provide a clearer specification for how the dependencies should be interpreted for each version of the plugin. The three types of constraints that matter are Cordova CLI version (easiest for third party developers to map against), platform versions, and plugin versions (as is done in plugin.xml's dependency tag).

I don't think that the CLI version provides the most reliable information because how easy it is for the CLI to go out of sync with a project (e.g. by updating it or a project's platforms), but after discussing this offline I have been convinced that third party plugin developers will have an easier time testing against a CLI as opposed to individual platforms.

I also added the option for specifying breaking platform changes for all earlier versions of a plugin (see the <plugin-version> section). Hopefully that should make maintenance much easier.

@stevengill
Copy link
Contributor

This looks pretty good! Logic looks sound to me.

Question: Why use a custom attribute (cordovaDependencies) instead of the engine attribute in package.json?

@dblotsky
Copy link
Contributor

The engine attribute in package.json is for Node, isn't it? Is it conventionally ok to specify other things there?

@stevengill
Copy link
Contributor

I've seen it used for other things (npm, iojs, etc). Conventionally, it would be encouraged to reuse the engine tag. A lot clearer than cordovaDependencies.

@riknoll
Copy link
Contributor Author

riknoll commented Jan 15, 2016

That's fine with me!

@riknoll
Copy link
Contributor Author

riknoll commented Jan 20, 2016

Looks like the engine tag might require that you give a node version: https://docs.npmjs.com/files/package.json#engines

If that is the case, we might have to put it somewhere else since it doesn't really make sense for plugins to specify that version.

@stevengill
Copy link
Contributor

I don't think the engine tag requires node. The docs just show npm looks for node, iojs and npm. But we can put whatever we want in there.

I made a quick test: https://github.com/stevengill/enginesTest

I figured you would still want a named object to grab all of the versions with one call. So I put everything in a cordovaDependencies object.

"engines": {
      "cordovaDependencies": {
        "0.0.1": { 
            "cordova-ios": ">1.0.0",
            "cordova": "~4.0.0" 
        },
        "<1.0.0": { "cordova-ios": "<2.0.0" },
        "<2.0.0": { "cordova-ios": "<5.0.0" }
      }
  }

I'm fine if you want to keep it in cordova instead of engines. I just think it makes the purpose of it more clear being in engine

@riknoll
Copy link
Contributor Author

riknoll commented Jan 21, 2016

If that works, I'm all for it! The npm docs are really unclear about that fact.

@stevengill
Copy link
Contributor

Can I merge this PR in now and close it?

stevengill added a commit that referenced this pull request Mar 18, 2016
Adding plugin version fetching proposal
@stevengill stevengill merged commit 5542c06 into apache:master Mar 18, 2016
@stevengill
Copy link
Contributor

I merged it in. Wanted to reference it in blog post

@riknoll
Copy link
Contributor Author

riknoll commented Mar 18, 2016

@stevengill I wrote up something for the blog and some actual docs here. Sorry, I meant to send out a link to that. Let me know what you think!

@stevengill
Copy link
Contributor

@riknoll oh good to know! I can just reference your post instead of the proposal in the tools release blog post. apache/cordova-docs#561

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

Successfully merging this pull request may close these issues.

None yet

3 participants