Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[eslint] Forbid importing common code to public or server across plugin boundaries #127829

Closed
lukeelmers opened this issue Mar 15, 2022 · 6 comments
Labels
DX Issues related to Developer Experience impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@lukeelmers
Copy link
Member

In #118826, @mattkime proposed a convention for differentiating between private & public code in a plugin's common directory.

One thing that came up in the discussion was that we currently aren't putting any restrictions in place for when a plugin imports from a common directory that lives in another plugin[0]. The main reasoning for this is, presumably, that we have the extraPublicDirs setting in the plugin manifest (see #68986), and as a result plugins can technically do static code imports from any directory in another plugin.

However, extraPublicDir has been deprecated from the beginning as a temporary solution, and we intend to ultimately get rid of it, perhaps eventually formalizing common directories as a first-class concept.

We should consider adding eslint rules to enforce the following:

  • forbid code in a server directory importing from another plugin's common directory
  • forbid code in a public directory importing from another plugin's common directory
  • allow code in a common directory importing from another plugin's common directory
  • allow server or public code importing from its own common directory

The tradeoff here would be that plugins would need to ignore eslint when importing from another plugin that is using extraPublicDir to expose code from a common directory. IMO that's an acceptable tradeoff, especially as we are looking to sunset extraPublicDir anyway.

But there may be something I'm overlooking here. Are there any other downsides I am missing?

cc @stacey-gammon @spalger @tylersmalley


[0] Based on comments I'm reading in other issues, I think historically there has been assumption that we have restricted this behavior, but it looks like this is no longer the case.

@lukeelmers lukeelmers added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc loe:small Small Level of Effort impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. DX Issues related to Developer Experience labels Mar 15, 2022
@elasticmachine
Copy link
Contributor

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

@afharo
Copy link
Member

afharo commented Mar 16, 2022

allow code in a common directory importing from another plugin's common directory

Are we OK with allowing this? Could it create circular dependencies?

@stacey-gammon
Copy link
Contributor

Why aren't we okay with server -> common or public -> common? I could see us linting on the inverse (don't allow common code to import from server code), but I don't understand why this is problematic.

@mattkime
Copy link
Contributor

Why aren't we okay with server -> common or public -> common? I could see us linting on the inverse (don't allow common code to import from server code), but I don't understand why this is problematic.

IMO its mostly a clarity concern. For me, either we don't allow importing from other plugin's common directory OR we have some way of determining between whats being exported for inter vs intra plugin usage.

@spalger
Copy link
Contributor

spalger commented Mar 16, 2022

I don't have much input regarding ESLint rule changes for the current state of things, I'm thinking a lot about the near future state where everything is a package:

The shape of this problem is going to change when everything is a package. Packages won't have separate entry points like /public /server and /common but we will need to use kibana.json or something to describe the intent of the package and validate its usage. In that case I think we would want to focus on "server/browser compatibility" and the "package private" usage. A package might be usable on both the server and the browser but if it's intended to only be used by specific other packages the kibana.json should probably be able to declare that and have it enforced by an ESLint rule.

@pgayvallet
Copy link
Contributor

Outdated problem, closing

@pgayvallet pgayvallet closed this as not planned Won't fix, can't repro, duplicate, stale Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Issues related to Developer Experience impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

7 participants