[Spec] Merge quickly, then stabilize

[davidortinau]

Problem

Pull Requests sit for way too long in review or draft because of the high bar of committing to the API and implementation.

Examples and candidates to consider:

• MediaElement [Android,iOS,UWP,WPF] MediaElement control #3482
• CameraView [iOS, Android, UWP] CameraView control #4084
• GIF [Enhancement] Basic GIF animation features. #2202
• Shell for UWP Adds UWP support to Shell #5936

Suggestion: SetFlags

Could we use the SetFlags API to merge these kinds of controls before they meet the bar for stable, but still ship them with stable releases? This would allow more aggressive developers to opt-in with the expectation that stability may not be there, and the API/ABI could change even radically.

Concerns:

• Code touching Core runs the risk of impacting other controls
• Companies may not be able to adopt current stable releases when regulations stipulate that all features in a release meet a certain standard of stability/quality.

Suggestion: Fast Ring, Slow Ring

Could we maintain 2 shipping lanes? Thinking in terms of both the stable and pre-release categories we now have, within each we could have:

• Slow Ring: includes stable or pre-release
• Fast Ring: includes stable or pre-release PLUS anything like the items noted above that will be included, but are being actively developer or finalized

Concerns:

• Added burden on the core team to maintain this release process
• Added burden on the core team to merge all the things to make this work.

Suggestion: Close Them

Not my preference, but we could close them or reject them quickly and suggest they exist as separate NuGet packages. Or we could direct them to the Xamarin Community Toolkit.

Concerns:

• While some developers are fine with adding 3rd party NuGets, for many companies the requirement to have the MIcrosoft ownership of something is critical.

Suggestion: Close Them, But Own the NuGet

To the concern above about ownership/support, the core team could take a feature as a separate NuGet that we own. We would need to add guidance and requirements (approved dependencies only, etc).

Concerns:

• More projects, more fragmentation...the opposite of the Essentials initiative
• More NuGets, potential upgrading nightmares
• More NuGets, potential performance impact

Call to Action

Please discuss and contribute your ideas here.

[migueldeicaza]

I think we need different policies for the api surface vs the implementation.

The former is problematic because we cause frustration when we break the API, and that deserves to be at least get an API review, without trying to boil the ocean. This could be set behind a flag, but ideally if the baseline API is fine, we only hide or do not expose debatable API entry points and evolve those separately.

The implementation I feel is more flexible. If the code is of acceptable design and quality, we should merge right away and work towards getting it released and published and used as much as possible.

This latter point is key: we don’t hide behind a feature flag, it make it easy to try, as this is how we get the testing required to harden the API.

For the first case, I propose that we set a weekly cadence to review issues and unblock and give clear guidance for merging. And that no PR is held back for more than some days without clear direction on API changes.

[ChaseFlorell]

I like the "fast ring" "slow ring" idea.
What about a simple compiler directive for each and ship them separate. Then I as a contributor can still ship my control but have the small requirement to ensure it lands in the "fast lane" only build.

[Conditional("Fast_Ring")]
public class MyNewControl : VisualElement
{

}

When it comes to work that's not net new, it can become more tricky. If it's (for example) a brand new property, the Conditional attribute still applies. When it comes to changes to existing functionality being changed... what do you do then? How do you evaluate if it's a breaking change?

// might be a maintenance nightmare

[Conditional("Slow_Ring")] // only required if there's a Fast_Ring alternative
private void MethodThatDoesSomething()
{
    // old way
}

[Conditional("Fast_Ring")]
private void MethodThatDoesSomething()
{
    // new way
}

[charlesroddie]

Good that this problem is getting some attention.

Creating nuget packages from PR branches would help the review process, allowing affected users to test for functionality and API, and lowering the bar of expertise required to provide helpful feedback. This could be automated. Someone comments "release please" on a PR branch and that releases an automatically named package? Would that be easy for consuming code to switch to even if it's not the main Xamarin.Forms nuget?

Doing this via fast rings would require more effort, to maintain the fast branch. And users who want to test a particular PR aren't the same as users who are generally interested in what is currently being worked on. The current state of regressions wouldn't encourage users to be adventurous with fast branches.

Even with a good process, there will be a need for more effort in getting PRs in. The basics of maintenance need to be prioritized over exploring all sorts of new territory. A cultural issue with Xamarin & mono IMHO.

[nickrandolph]

Feedback on the suggestions:

Suggestion: SetFlags
Don't do this - this adds significantly to any test matrix (ie testing every combination of setflags) and as pointed out in the description prevents companies adopting XF if they need a "stable" release.
Selecting a version of XF from the main NuGet feed should include only those features that have been released, not experimental features.

Suggestion: Fast Ring, Slow Ring
Again, don't do this.
What you should be doing is moving to a release each time a PR is approved to master (ie code that's ready to ship). If code has been merged to master it should be considered shipped and there should be a corresponding NuGet package for it. The downside of this is that there will be a large number of NuGet packages, making it hard for developers to know whether to adopt each package - this can be dealt with by explicit about what changes in version numbering means.

Suggestion: Close Them
Yes, make a decision quickly whether a PR should ever been accepted. For those that aren't, the author(s) need to work out where the code should be taken. No longer an XF team's responsibility.

Suggestion: Close Them, But Own the NuGet
No, don't fragment the XF space any more.

An additional thought:

• Make use of draft PRs - essentially any PR that hasn't met the basic guidelines should remain as a draft until the author(s) are able to attend to any outstanding issues. Once they're satisfied, the PR can be made non-draft, and the XF team can do a review pass. If the PR still can't be accepted, it gets returned to draft with additional acceptance criteria added. This cycle can continue until the PR is approved.

The big thing here is timeliness to get PRs actioned. I like what @migueldeicaza suggested about having a weekly (and ideally the time of this meeting should be widely know and anticipated) meeting to move active PRs forward.

• Make use of package creation from working branches. For example the iterations on Shell, should be done on a separate branch, and only when it's ready to be released should it be merged to master. In the meantime, there can be progressive packages generated from the Shell branch so that those who are tracking the ongoing development can pull the latest packages.

I think this Spec should look at the wider topic of the way that the XF team interfaces with the community and how features get proposed and developed. As we're seeing with Shell, the lack of early engagement with the community, is now causing things like navigation and third party library integration to have to be iterated over.

[dotMorten]

Not sure why you specifically call out #5936. It's still a draft release as I'm working through some issues. However I did hope for some guidance whether I was on the rigth track or not...

[adrianknight89]

I favor fragmentation with the caveat that MS owns the fragmented pieces. In the past, I've used projects that were so out of date yet the only alternatives out there. I like the work the Essentials team is doing. As @davidortinau said, fragmentation is the opposite, but at some point we all realize we can't dump everything in the Essentials project, especially things that involve UI. From my understanding, this project is intended to be a collection of small dependency-service like functionalities. A video or image viewer should not be part of it.

Personally, I don't like the SetFlags or fast/slow ring options. I'm in favor of closing PRs that would become a burden for the Forms team to maintain and delegating this responsibility to another MS team that owns their own NuGet. In order to achieve this, Forms needs to be more extensible than it is now (e.g. get rid of internals and make things virtual).

Having said that, I'd love to see Microsoft support official packages for image and video. They would have image support for caching, GIF, transformations, SVGs, fluent API, error handling and video support for live streaming, volume control, playback status, and so forth. These libraries are not easy to create and maintain, and any community effort out there is inadequate.

[adrianknight89]

Has the team reached any consensus as to how long-sitting PRs should be treated? Also, this spec seems more focused on features than bug fixes. We need to be able to quickly merge bug fixes (~50% of active PRs) as well.

[samhouts]

We're starting to do this more and more with flags.