Style Decision: Namespacing functions with class/static

[stacey-gammon]

I have a personal preference for namespacing functions by putting them inside a class and marking them as static. Ex:

export class DashboardStrings {
  static getUnsavedChangesWarningMessage(changedFilters) {...}
  static getDashboardTitle(title, viewMode, isDirty) {... }
}

I've checked in code like this in a couple places:
https://github.com/elastic/kibana/blob/master/src/core_plugins/kibana/public/dashboard/filter_utils.js
https://github.com/elastic/kibana/blob/master/src/ui/public/utils/string_utils.js
and have more code in a view/edit PR like this (#10585)

The other common way of doing this in our code base is by exporting each function individually:

export function getUnsavedChangesWarningMessage(changedFilters) { /* ... */ }
export function getDashboardTitle(title, viewMode, isDirty) { /* ... */ }

I'd like to poll the team to see if there is a strong consensus for doing it one way or the other, so I can be consistent if there is.

The reason I prefer the namespacing:

• Avoids naming conflicts.
• Provides additional context.
• Easier to locate the file and location of the function when the exported module name matches the file name.
• Easier to locate all places the function is being used, especially when there are similarly named functions. E.g. searching for DashboardStrings.getEditPath as opposed to getEditPath which could be a function used in many different contexts.
• Import only one class and then view all available functions via auto completion, as opposed to navigating to that file to see what else is available.

@kjbekkelund gave some reasons to prefer exporting the individual function:

• You still have a namespace (the file), so you can for example import * as DashboardStrings from './file' (this will also give you autocompletion)
• It gives you the opportunity of only importing the one function you need (for this specific use-case that's not necessarily relevant, but for other utils it might be). In general I really like being able to import { utilFn } from './someUtilFile'.
• I haven't seen this pattern using class + static in JavaScript before. Not necessarily a relevant point at all, just an observation.
• If you push this further to a lib perspective — so not really interesting in the Kibana perspective, but more for the higher-level reason for my opinion — doing this enables tree-shaking (aka dead-code elimination). Again, not necessarily relevant for Kibana, just why this is perhaps a more used approach in the JS world.

If you prefer the class/static namespacing, 👍 this issue. If you prefer exporting functions individually, 👎 this issue. If you have another preference, comment, and if you don't care either way, then you can go about your day. 😸

[epixa]

The static function thing seems to be overly restrictive, so we'll probably end up having a bunch of special cases for when we don't want to use static functions in classes. For example, it's not clear to me how to reconcile static functions on a class with traditional stateless react components, unless we created a bunch of classes each with only a single static function, which would probably be named component or something like that?

I think better guidelines around destructured imports would likely address most of the pros you list in favor of the class approach.

[spalger]

I'm a big fan of keeping modules pretty limited and generally aim for a single function/class per module. I would find it very annoying if I had to wrap each of those functions in a class and address then as UniqueClassNamePerFile.functionName.

[stacey-gammon]

Thanks everyone, individual exported functions for the unanimous win! Much appreciate the input!

[epixa]

This was an interesting experiment, by the way. With pretty low investment from the team, you were able to turnaround asynchronously on a decision based on consensus for a code style idea.

It may break down for larger or more contentious discussions, but it's great starting point in general. Had this been contentious, we would have seen a lot of back and forth in comments and could have brought the discussion to zoom or something, and we'd still have a concrete proposal like this to provide context as well as somewhere to log a decision for future generations. It also gives the community an opportunity to participate.

We should do this more for concrete proposals like this.