[SR] Refactor proposal

[sebelga]

I created this PR as it was easier for me to express the changes I'd propose on the merged PR #33367

I added 5 commits that could be reverted separately if we totally disagree on them 😊

---

1. Move app dependencies to its own context provider

I think we need to separate the react app dependencies (core and plugins) from the app state. We don't want at any time a context consumer to modify the dependencies and a "state" by definition can and will be modified as the application changes.

2. Add barrel file for common types

It is a good practice to have a barrel file that exports all the types in a folder and it makes import easier (shorter path and no need to remember from what subfolder a type belongs).

3. Move Enums to constants file

As enum are values and not types, I moved them to the constants.ts file

4. Refactor function component using React.FunctionComponent<Props>

The correct way to type a function component is using the React.FunctionComponent so we'd benefit from any future update on that interface.

5. Refactor services folder

I felt like the current services folder was a mix of differents "things" not related and I am afraid as the app growth it will end up being a "drop anything" there that is not either a type, component or constant. So I added some structure to the folder by separating by "area". Then inside each area we might have a utility function, a serializer, a hook, a context... All of them exported from its barrel file.

---

If you want we can discuss those commits over zoom, it might be easier to share our thought process.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[jen-huang]

LGTM - I don't have any objections to this proposal 😁 I especially like the term AppDependencies, and the separation of areas in services directory. Thanks a lot for doing this!

[elasticmachine]

Pinging @elastic/es-ui

[cjcenizal]

Nice improvements @sebelga! Just had a couple questions but nothing that needs to block this from being merged.

[cjcenizal]

I don't think this belongs in common since it's mean only to be consumed by client-side code. Can we move it into a constants file in the public directory?

After looking deeper, I think this applies to REPOSITORY_TYPES as well.

[jen-huang]

I think REPOSITORY_DOC_PATHS should be moved to public, but there's a good chance that REPOSITORY_TYPES will be needed server-side at some point so I'm fine with keeping it in common

[sebelga]

@cjcenizal @jen-huang My "please remove all frictions" brain tells me: why bother trying to separate constants file according to where they are consumed from. It adds a (small) layer of complexity that I don't think brings anything. When we import a constant we have to go to the following thought process:

• Is this constant used also on the server?
• Ummm I don't think so.... (So we go to the "public" folder, looking for the constant)
• Oh! I forgot, we updated last week and the server also requires it (and we have to go to the "common" folder check if the constant is indeed there)

Instead of:

• I need a constant... cool, let's import it from <app>/constants/index.ts

No need to update anything if suddenly a constant changes context.

[cjcenizal]

I see what you're saying, Seb! From the point of view of someone writing code, it's easier to know all constants live in one place. In contrast, I think I tend to optimize for the DX of someone reading the code. Let me see if I can explain my perspective a bit, and why I think breaking things up makes the code simpler and easier to read.

Code written for the client has a different context than code written for the server, e.g. server-side code has access to Node APIs and client-side code has access to Web APIs, client-side code can use webpack aliases to import from public/ui and server-side code can't. Code which is written to be used both for the client and the server has to adhere to the restrictions of both server-side and client-side code, and can also by its very nature be used in more places than what's written just for the server or the client. When I'm reading code, all of these "rules" factor into my mental model of how the code works, its relationships in the dependency tree, and where it will run.

Code organized into server-side or client-side buckets is easier for me to understand, because the domains are clearly defined and the rules for each are limited. common code is much more difficult for me because the domain is much broader, involves more rules, and is therefore more complex. It's sort of like putting variables into the global scope -- making changes to that code becomes more difficult because you're not sure what's using it.

Because of this, I'd strive to localize constants are closely as possible to where they're consumed. If constants are only used inside of a single file, then I'd keep them defined in that file. Once we discover a need to share them with another module, I'd extract them out into an area where they can be accessed more broadly, like a constants directory. Continuing in that direction, once we discover a need to share them between both client and server, I'd move them into common. This way the location of the code communicates information about what may be using it.

[sebelga]

Thanks for detailing your view on this. Obviously, if there are rules about imports and so, it makes sense to not mix client with server code. I was referring to files simply exporting strings and values without any domain logic or references outside the scope of the file.

I guess we are in the "move it until it feels right" tweet that @jen-huang shared the other day 😊 As I get older (!) I tend to try to simplify the decision we're talking here. I can give you another example: when I was going through the rollup (and even the ccr) app, a few times I had to think: "in which folder is that component?" because of the way folder were structured. I had to remember who was its parent (in the DOM!), so I could go there and look for a subfolder to find the component. I understand "why" it's been put there, I used to do that a lot. Now I lean more towards removing all the need of thought process to find/import something and having a flatter structure when possible (as EUI does with its components).

You're going to ask me: "so when do we start to organize things into folders"? Well, that's the sweet spot that is difficult to get right, especially with multiple devs involved. In my personal project, I keep moving things around! 😄 so no wonder discussions like this one pop up from time to time in a team...

[cjcenizal]

Have we checked with the platform team that it makes sense to use webpack aliases to import these things in a non-shim file? My understanding was that we wanted to isolate all external imports within the shim, but this seems contrary to that guidance.

[jen-huang]

good call CJ, we shouldn't be importing these outside of shim.ts. maybe we can move these back into shim but re-export them here

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request