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

Re-use global plugins inside shareable configurations #1309

Open
dominykas opened this issue Oct 8, 2019 · 16 comments
Open

Re-use global plugins inside shareable configurations #1309

dominykas opened this issue Oct 8, 2019 · 16 comments

Comments

@dominykas
Copy link
Contributor

dominykas commented Oct 8, 2019

New feature motivation

At the moment, when using a shareable configuration which has a plugins array, semantic-release expects the plugins to be the dependencies of the shareable configuration.

If you keep semantic-release globally installed (or use npx) and the shareable configuration installed per project, you will be installing the default (npm, github, etc) plugins twice. This slows down the build/release process significantly.

New feature description

I can see three ways this can work:

  1. If a shared configuration is using one of the default plugins, it should be resolved relative to semantic-release, not relative to config.
    • I can see this as being desirable, as the version bundled with semantic-release is the "official" version, whereas the version relative to the config could be outdated/incompatible (ah, peer dep management)
    • I can see this as being undesirable, as people should be able to use specific versions that they want
  2. If a plugin is installed relative to the shared configuration - use that, otherwise - try to resolve relative to semantic-release.
    • I think this is probably my own preference, as it is automated, and I think there's already a case for similar fallback, where loadPlugin tries to resolve relative to cwd as a fallback.
  3. Make it configurable, by e.g. marking the plugin as { local: false }, which would then force it to be resolved relative to semantic-release.

New feature implementation

Depends on the path chosen.

Happy to PR if there's agreement.

Workaround

The workaround is to provide the full path to the plugin. The path can be resolved relative to the "main" module, e.g.

const resolveFrom = require('resolve-from')

const plugins = [
  resolveFrom.silent(path.dirname(process.mainModule.filename), '@semantic-release/npm')
];

Not pretty, but it does work.

--

Edit: added the workaround

@travi
Copy link
Member

travi commented Oct 8, 2019

i think the behavior i would expect if i was pulling in a shareable config would be (2) since the dependencies of the config would in theory be tested to be compatible with each other and not expect others that happen to be included to accidentally override.

this situation is similar to the hoisting problem that is being discussed in an rfc for eslint, so that may provide some context that could help with the decision around how this should be handled.

i also wonder if the fact that semantic-release includes plugins by default influences enough to make this a different situation than what is happening with eslint (in my mind the current situation actually makes it harder to get the shareable config to work the way i would expect) and if that difference changes if something along the lines of #1260 is pursued at some point.

@pvdlg
Copy link
Member

pvdlg commented Oct 8, 2019

I'm pretty sure this situation is already handled and plugins are loaded locally and if absent they are loaded globally using cwd as the base for search:

function loadPlugin({cwd}, name, pluginsPath) {

@dominykas
Copy link
Contributor Author

dominykas commented Oct 8, 2019

The cwd case is handled, but semantic-release may be installed globally, not in cwd (which is likely the git root of the package).

@pvdlg
Copy link
Member

pvdlg commented Oct 8, 2019

If semantic-release is installed globally then resolveFrom.silent(__dirname, pluginsPath[name]) will find the globally installed plugins.

@dominykas
Copy link
Contributor Author

No, because pluginPaths[name] is an absolute path, IIRC (not at laptop right now)

@pvdlg
Copy link
Member

pvdlg commented Oct 8, 2019

pluginPaths[name] will contains the path of the shareable config from which the plugin is referenced.

If you have a locally installed shareable config we expect the plugin to be locally installed.
If you have a globally installed shareable config we expect the plugin to be globally installed.

This is because we expect the shareable config module to define the dependencies it needs.

@pvdlg
Copy link
Member

pvdlg commented Oct 8, 2019

I don't think we will modify the way we load plugin and shareable config as it's already complicated enough. Here a few reasons:

  • Having semantic-release installed globally and the shareable config installed locally is at best a very edgy case. The goal of shareable is to make them shareable....via npm module...I don't see why anyone would install locally when they install semantic-release.
  • As edgy as it might be, the case you describe works.
  • The benefit would only be to save a few milliseconds/seconds on the first install (when there is no npm local cache)

Unless I'm miss something, increasing the already complex plugin system for so few benefits doesn't seems a great trade-off.

@pvdlg
Copy link
Member

pvdlg commented Oct 8, 2019

As a side note the plugins are installed by npm based on dependencies in the package.json. semantic-release has no impact on what is installed where, it's all handled by npm. We have an impact only on where we look for in order to load plugins.

So no matter what we would do in the semantic-release that wouldn't prevent to install plugins twice if you depends on them both locally and globally.

@dominykas
Copy link
Contributor Author

dominykas commented Oct 8, 2019

Having semantic-release installed globally and the shareable config installed locally is at best a very edgy case.

I would definitely not call this an edgy case. It makes perfect sense for a large team to have a global install of semantic-release available on their CI server to speed things up. And then if you have sub-teams, with different needs, they can all have their individual shared configs. It is unreasonable to expect the CI server to have all of these configs installed globally (it's a shared server after all - so access is likely controlled).

The other use case I have is bundling the shared commitlint config into the same package (because commitlint and semantic-release need to have the same commit rules). I would not underestimate the extra burden of having to manage extra dependencies across a number of repos, when bundling the logic makes sense.

The benefit would only be to save a few milliseconds/seconds on the first install (when there is no npm local cache)

Installing just the @semantic-release/npm takes ~10s on my machine with warm cache. Installing full semantic-release takes ~30s. It adds up, esp. if you have to release multiple repos. You also have to install these things even for PR builds or your local development, i.e. when you're not even going to release.

Unless I'm miss something, increasing the already complex plugin system for so few benefits doesn't seems a great trade-off.

It's a one line change, as I have demonstrated in the workaround.

@travi
Copy link
Member

travi commented Oct 8, 2019

Having semantic-release installed globally and the shareable config installed locally is at best a very edgy case.

the case where i could see realistically doing this myself is where the shareable config is installed locally, but semantic-release is run with npx but is not installed locally.

to handle this in the past, i've installed the shareable config globally. that seems like the more correct way to handle this, but i could see what i initially described as being arguably more ergonomic. not necessarily saying that's enough of a reason to support it though.

@pvdlg
Copy link
Member

pvdlg commented Oct 8, 2019

In any case, as mentioned here changing the code in semantic-release will not solve your problem.

It's not semantic-release that install the dependencies, it's npm. So if you have @sematnic-release/npm as dependency of the global semantic-release and as a dependency of the local config it will be install twice and there is nothing we can do about.

The workaround you proposed just influence were we load the plugins from not were npm install them. They will be installed twice and we would just load the other one.

@dominykas
Copy link
Contributor Author

dominykas commented Oct 8, 2019

They will be installed twice and we would just load the other one.

They will only be installed twice if they are included in dependencies of the shared config.

The whole point I'm trying to make with this issue is that there should be no need to install them twice or include them in dependencies as part of the shared config, if the shared config considers that it's OK to use the default ones that come with semantic-release.

It's the same as if you have release.config.js in your repo, and you use plugins, and they just work with the globally installed semantic-release without having to include them as deps in your repo.

@pvdlg
Copy link
Member

pvdlg commented Oct 8, 2019

We want to have the shared config to specify their dependencies so they can choose a specific version.

@dominykas
Copy link
Contributor Author

Being able to chose a specific version and being forced to chose a specific version are two different things?

@travi
Copy link
Member

travi commented Oct 8, 2019

from my comment above:

the dependencies of the config would in theory be tested to be compatible with each other

this is the key piece for me, and i think what @pvdlg is trying to highlight. if the shareable config defines it's own dependencies, it is able to control which version range it is compatible with. by using versions that are not provided by the config, the risk is introduced that a breaking change in the provided plugins breaks the config.

this is why my expectation is that the config always defines its own dependencies, at minimum as a peer-dependency. npm ls can then be used to enforce that appropriate versions are installed. if going as far as defining a peer, i see no reason not to actually define as a full-blown dependency.

It's the same as if you have release.config.js in your repo, and you use plugins, and they just work with the globally installed semantic-release without having to include them as deps in your repo.

this might be the disconnect? by the nature of extending a shared config, my expectation is that extends the config as a whole, including its dependencies, rather than just extending it as a config file with packages installed by other means.

@dominykas
Copy link
Contributor Author

this is why my expectation is that the config always defines its own dependencies, at minimum as a peer-dependency.

Sure, I understand this bit and it is valuable. However npm does not really help here (well, at least until v7, when it will start trying to install peerDeps again, which means that defining peerDeps will bring us back to where we are now - we'll get two copies installed) for the cases where package is installed in separate places (global vs local).

As an example, hapi moved away from defining peer dependencies, and instead the plugins define the hapi range they are compatible with, while the framework enforces that. This does somewhat overlap with npm's functionality, but it does offer rigidity.

Probably out of scope here, I can open a separate issue to discuss this if this is interesting.

this might be the disconnect? by the nature of extending a shared config, my expectation is that extends the config as a whole, including its dependencies

I don't see this as mutually exclusive? You can still do that - I'm just asking provide a smart fallback to reduce friction?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants