[discuss] Need shared pattern for plugin private common code

[mattkime]

The problem -

We have three directories we export from - public, server, and common. Code exported from public and server can only be consumed by other plugins but code exported from common may either be consumed by other plugins OR may be exported for internal use by public or server functionality. Currently, as best I can tell, we don't have an agreed upon pattern for drawing a distinction between the two intentions.

The proposal - common/index.ts will be exports for other plugins. common/private.ts will be for internal use.

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[pgayvallet]

This is just about a pattern right? We're not expecting to allow plugin/server/foo.ts to import things from common/private.ts via import { stuff } from '../common, it will have to explicitely import from ../common/private, right?

If that's the case, I'm personally fine with it. I would change private.ts to internal.ts though, to better match the @internal annotation we've being using (at least in the core team)

[mshustov]

but code exported from common may either be consumed by other plugins

This is forbidden by @kbn/eslint/no-restricted-paths eslint rule

kibana/.eslintrc.js

Lines 514 to 529 in 9a8499c

	target: [
	'(src|x-pack)/plugins/**/(public|server)/**/*',
	'examples/**/*',
	'!(src|x-pack)/**/*.test.*',
	'!(x-pack/)?test/**/*',
	],
	from: [
	'(src|x-pack)/plugins/**/(public|server)/**/*',
	'!(src|x-pack)/plugins/**/(public|server)/mocks/index.{js,mjs,ts}',
	'!(src|x-pack)/plugins/**/(public|server)/(index|mocks).{js,mjs,ts,tsx}',
	'!(src|x-pack)/plugins/**/__stories__/index.{js,mjs,ts,tsx}',
	'!(src|x-pack)/plugins/**/__fixtures__/index.{js,mjs,ts,tsx}',
	],
	allowSameFolder: true,
	errorMessage: 'Plugins may only import from top-level public and server modules.',
	},

We didn't want to introduce another environment and wanted plugins to import only from /server or /public. We don't enforce this rule in the runtime though. cc @elastic/kibana-operations

[mattkime]

Okay, I'll take a closer look at this, perhaps I was mistaken.

[mattkime]

Either I'm confused or I see quite a number of exceptions to this - https://github.com/elastic/kibana/pull/126315/files

[lukeelmers]

@mattkime As Mikhail pointed out above, the intent for sharing static code has always been that we should only be importing from public/index.ts or server/index.ts. The main use case for allowing imports from common/index.ts would be when you are writing code that lives inside your plugin's common directory, and therefore can't specifically import from public/server.

Other than that scenario, I'd recommend avoiding cross-plugin common imports, including in the PR you are referencing above.

As for how to distinguish between shared and internal common code, I think this is mostly determined by the extraPublicDirs option in the kibana.json. If you have added "extraPublicDirs": ["common"], then the assumption is that everything exported from your common/index.ts should be considered public, and therefore any internal/private items should be exported separately. If you have added "extraPublicDirs": ["common/foo"], then any items in common/foo should be considered public, etc.

The mechanics of how you export private code is an implementation detail of your plugin, and I think there are several viable options -- including common/internal.ts, or even just doing nested imports for items that aren't in the publicDir, e.g. common/some/internal/thing.ts. How to handle that is mostly a matter of convention, and I don't think we on the core team have any strong opinion on it -- our main concern is just that you aren't exporting any internal code from a publicDir.

cc @stacey-gammon

[mattkime]

@lukeelmers I guess the @kbn/eslint/no-restricted-paths rule is informed by "extraPublicDirs": ["common"] which I wasn't aware of. Thanks.

[lukeelmers]

I guess the @kbn/eslint/no-restricted-paths rule is informed by "extraPublicDirs": ["common"] which I wasn't aware of.

I actually don't think it's that we are taking extraPublicDirs into account, but rather that we aren't applying this eslint rule to imports that don't include public or server in the path. So eslint will let you import from any subdirectory of common without complaining, but it'll complain if you try to import from a subdirectory of public or server.

[mattkime]

Its clear that at very least we should remove imports from common of the sort used by dashboards.

Past that, I'm confused why dashboards common imports are allowed. As stated above -

but code exported from common may either be consumed by other plugins

This is forbidden by @kbn/eslint/no-restricted-paths eslint rule

clearly, this isn't true and I don't understand why.

[stacey-gammon]

Thanks for tagging me @lukeelmers. I would actually like to see extraPublicDirs deprecated - see #101948.

I don't see anything wrong with @mattkime's original suggestion, though sounds like we'd have to fix the lint rules to allow deep imports from the same plugin's server and public dirs.

cc @spalger

[lukeelmers]

Past that, I'm confused why dashboards common imports are allowed.

@mattkime Yeah, I think the reason why our eslint rules don't currently enforce this is precisely because of the existence of extraPublicDirs, which technically allows you to share any path with other plugins, even if it isn't top-level.

sounds like we'd have to fix the lint rules to allow deep imports from the same plugin's server and public dirs.

The existing lint rules should already allow for this.

e.g. if I am in src/plugins/foo/server/file.ts, I can import from ../common/another/file.ts

I would actually like to see extraPublicDirs deprecated - see #101948.

Agreed 🙂 Spencer marked this setting as deprecated from the beginning in #68986, with the understanding that this was only ever intended as a temporary solution as we worked on better ways of sharing static code. But I think we could instead enforce that sharing of static code only happens from {server|public|common}/index.ts as you suggest in that issue. I'm not sure if there's a technical reason why we'd need to keep the setting if we were to do that; @spalger may know otherwise.