[Discuss] Code alignment between master and 7.x branches

[sebelga]

In the ES UI team, we have several places (upgrade assistant, mappings and now the upcoming #78331) where the code between master and 7.x diverges.

Those differences are a problem for

• backporting (poor DX and risk of regression during merge conflict)
• potential bugs (we forget to test 7.x and we got Saving not possible after editing index template #78384)

We could try to remember all the little nuances between the branches, but we know we will forget them as we keep adding functionalities and plugins. We can try to keep a document up to date with the differences but I think it will get quickly obsolete and hard to parse to get a full picture of the differences.

The best place to indicate the difference IMO is in the code itself.

if (BRANCH === '7.x') {
  ...
}

We can't get much clearer than that, and this can be backported without any problem.

In master, the above example will be evaluated to

if (false) {
  // this dead code will be removed by the JS minimizer and not part of the bundle
}

Dynamic imports

When possible, the code that will be deprecated (e.g. a component rendering a deprecated mapping field), should be exported from a barrel file following the convention: index_<branch>.ts.

As an example, the boost parameter is removed from the mappings in 8.x. We should remove its export from the current index.ts file and export it from a index_7x.ts file.
Then its import can be conditional, according to the branch we are on

let BoostParameter: React.FunctionComponent<any>;

if (BRANCH === '7.x') {
  import('../../field_parameters/index_7x').then((parameters) => {
    BoostParameter = parameters.BoostParameter;
  });
}

...and then in the JSX

{BRANCH === '7.x' && (
  <BoostParameter defaultToggleValue={getDefaultToggleValue('boost', field.source)} />
)}

Tests

As a side benefit, when the code is self-explanatory, then there is little risk to forget to add a test for that code branch.

if (BRANCH === '7.x') {
  test('it should have the boost parameter', () => { ... });
}

For now, I am only seeing the need of knowing the branch (the major version) the code will be run from. But this would leave the door open for feature flags in case there is a switch of priorities during a release cycle (which hasn't happened yet but we almost got into the situation in the last release).

[jloleysens]

I just wanted to highlight the Upgrade Assistant again as a plugin where the difference between master and 7.x is acute.

Code that only exists on 7.x

• server routes
• ui components
• bits of core logic

These all share the problem of not being discoverable from master, but I would say the last one, because it is nested inside of code that we reuse across versions, is the most troublesome. A mechanism like what you are describing can be very useful for making the core logic bits of UA easier to test and reason about!

I'm not quite sure what the best approach might be for the components and routes - their potential for merge conflicts and regressions seems smaller if the code is well contained to a file/module - but the trick of remembering to test (for visual or logic regressions) is still there unless good, automated tests exist.

Another piece I am foggy on, with having 7.x (and in future 8.x and 9.x) code alongside master, is how we handle going to the next major version. Do we simply delete the 7.x code from master when 7.x becomes 8.x? Or is there a way to leave the code in master and have the build assemble itself (again, tricky with points of overlap like the core logic bits in UA).

I suppose the if else logic can just continue to grow:

// but 👇🏻 branch might not be relevant to have around in future.
if (BRANCH === "7.x") {
} else if (BRANCH === "8.x") {
} else {}

[alisonelizabeth]

Thanks for starting this discussion @sebelga!

I also want to add that the Snapshot & Restore plugin is another example of where we have a divergence between branches for some server routes and associated tests. This is due to breaking changes in ES snapshot APIs that only target 8.x. This PR highlights the differences: #39533. This at one point did lead to a regression when we were working on NP migration.

[sebelga]

Another piece I am foggy on, with having 7.x (and in future 8.x and 9.x) code alongside master, is how we handle going to the next major version. Do we simply delete the 7.x code from master when 7.x becomes 8.x?

I would be in favor of cleaning any code branch we don't support anymore.

if (BRANCH === '7.x') {
  // remove this if
}

And when needed (where it is difficult to have a clear if { ... } cut), having a branch clean comment on top of the file, following a convention

// ----------------------------- 7.x branch clean -------------------------------------
// - Remove instance of "boost" in the switch statement inside getDefaultToggleValue()
// ------------------------------------------------------------------------------------

So when we need to clean we only need to make 3 searches:

• search for code containing "BRANCH === '7.x"
• search for code containing "7.x branch clean"
• search for file name ending in "_7x.ts(x)" (and delete those files and their respective imports)

[sebelga]

Update: I added an example for dynamic import in the PR description.

[kobelb]

I agree that this is a real problem that deserves a solution. With the 8.0 upgrade coming, it'd be great to start removing the deprecated functionality sooner rather than later without the risks that come with the master and 7.x branches diverging.

This is largely a nit-pick, but it feels awkward to use the "branch" to make this determination, it feels more natural for this determination to be performed using the actual version number of the product itself. For example, the following feels more natural to me:

if (version.before(Version.V_8_0_0)) {
  // allow deprecated functionality to proceed
} else {
   throw new Error('this is not unsupported');
}

This was the approach that I played around with a rather long time ago and abandoned: kobelb@00724de#diff-ce2a4faae344e68d6a0ec1fb307a9b2738446bc89685c78c1ca9552688754342

However, I haven't seen the code diverge enough between master and 7.x that it feels necessary to invest the effort to augment the build process to do "dead code elimination" and only include the version-specific parts in the bundles/distributable. Have you all come across situations where we'd be drastically increasing the size of the bundles/distributable by including the code for all versions?

[sebelga]

You are right that we could use a major version number instead of the branch name. 👍 Not sure we need the granularity of minors and patches.

As we get for free the dead code elimination with the js minimizer (all if (false) {} branches are removed), my proposal seemed to be the least effort to get it working.

Having a VersionService means more code to maintain and test for the author. And another service to import and inject for all dependant plugins.

[kobelb]

And another service to import and inject for all dependant plugins.

In my opinion, this is the biggest trade-off. However, it allows us to continue to use the version number specified in the package.json as the source-of-truth.

[sebelga]

Not sure how/when the "package.json" version is bumped but it seems that Webpack should be able to also import the file and inject the version. But again, I miss knowledge here on the order of operations.

[kobelb]

Is my understanding correct that you're advocating that we use Webpack primarily for dead code elimination? Most of the situations where I've wanted to do code-branching based on version number wouldn't have benefitted significantly from dead code elimination. Do you have any examples where this would have a significant impact on bundle size?

If dead code elimination isn't a deciding factor in the approach, we'll still have to decide between using a global-variable or having a "version service" which is provided by core. I generally dislike global-variables, to the extent that I avoid them wherever possible and would prefer the "version service" provided by core. Having the "version service" will make it easier to write unit-tests for code which does the version-based branching. However, the global-variable is potentially easier for code to consume without having to modify all of the code-paths to pass the "version service" from core to where it's consumed. Are there other benefits to the global-variable which you think are note-worthy, @sebelga?

It should also be noted that Webpack is only used for building the client-side bundles, so it won't be a native solution for the server-side.

[sebelga]

You're making a good point about global vars and unit tests, although it shouldn't be too hard to define them before executing the tests.

My main reason for Webpack was actually the simplicity and DX, before the benefit of dead code elimination. It seems that with the core services there are more moving parts and I don't see clearly why they are needed. If the only problem we need to solve is: is this branch X or Y (no need of the granularity of minor or patch version which the service could provide), then I was suggesting simplicity (both in implementation and consumption).

Regarding dead code elimination, you are right that there isn't currently a need to optimize for it. I just thought: if we have if for free, why not? :)

With that said, if you do want to avoid global vars and prefer the core service, I'm good with that, I let you decide. Just sharing some thoughts.

[alisonelizabeth]

@sebelga @kobelb Just wanted to follow-up on this as we start thinking about 8.0 UA work. As a next step, I can go ahead and open up an issue for the core team to consider implementing a "version" service if everyone is on board with that?

[kobelb]

@alisonelizabeth I'm on board! Thanks :elasticheart:

[alisonelizabeth]

Going to close this issue. I've opened #85969 to track the proposed implementation.