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

Enables all official plugins by default #4253

Merged
merged 25 commits into from
Mar 29, 2022
Merged

Enables all official plugins by default #4253

merged 25 commits into from
Mar 29, 2022

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Mar 22, 2022

What's the problem this PR addresses?

While the plugin architecture is useful to enforce proper separation of concerns, having our official features in multiple plugins isn't very good from a pure UX perspective and only enables smaller bundles. With Corepack becoming the recommended install strategy size is less of a concern (especially given the reasonably average size impact), so we can afford to add all plugins in the standard distribution.

This doesn't affect the plugin feature itself: third-party plugins will keep being supported, and there are no plans to stop providing the exact same API we currently expose.

How did you fix it?

Enables all plugins in the build config.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz merceyz added the major label Mar 22, 2022
@merceyz
Copy link
Member

merceyz commented Mar 22, 2022

What's the performance impact of loading all the plugins?

Copy link
Member

@paul-soporan paul-soporan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should automatically add @types/ dependencies by default. Since previously the plugin wasn't bundled, importing it served as a good opt-in check, but now we should probably use a heuristic to only add it if the project depends on typescript (though there should probably be a config option for it too so that people that really want a better editor experience without type-checking can force it too 🤔).

@arcanis
Copy link
Member Author

arcanis commented Mar 22, 2022

What's the performance impact of loading all the plugins?

I'll have to rework @yarnpkg/libui; the runtime cost is a little large at the moment:

Benchmark #1: YARN_IGNORE_PATH=1 node ./current.js --version
  Time (mean ± σ):     282.5 ms ±   2.0 ms    [User: 280.6 ms, System: 33.6 ms]
  Range (min … max):   279.5 ms … 285.0 ms    10 runs

Benchmark #2: YARN_IGNORE_PATH=1 node ./all-plugins.js --version
  Time (mean ± σ):     406.4 ms ±   3.8 ms    [User: 421.9 ms, System: 39.7 ms]
  Range (min … max):   402.7 ms … 415.5 ms    10 runs

Summary
  'YARN_IGNORE_PATH=1 node ./current.js --version' ran
    1.44 ± 0.02 times faster than 'YARN_IGNORE_PATH=1 node ./all-plugins.js --version'

@arcanis
Copy link
Member Author

arcanis commented Mar 23, 2022

It seems to be much more reasonable when taking v8-compile-cache into account (which is what Corepack automatically enables):

Benchmark #1: YARN_IGNORE_PATH=1 node -r $V8CC ./current.js --version
  Time (mean ± σ):     242.6 ms ±   3.2 ms    [User: 246.4 ms, System: 30.8 ms]
  Range (min … max):   238.7 ms … 247.2 ms    11 runs

Benchmark #2: YARN_IGNORE_PATH=1 node -r $V8CC ./all-plugins.js --version
  Time (mean ± σ):     341.0 ms ±   3.2 ms    [User: 359.5 ms, System: 38.6 ms]
  Range (min … max):   336.3 ms … 347.2 ms    10 runs

Benchmark #3: YARN_IGNORE_PATH=1 node -r $V8CC packages/yarnpkg-cli/bundles/yarn.js --version
  Time (mean ± σ):     274.2 ms ±  26.8 ms    [User: 274.3 ms, System: 34.1 ms]
  Range (min … max):   263.9 ms … 350.5 ms    10 runs

@arcanis arcanis mentioned this pull request Mar 23, 2022
13 tasks
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should print some kind of warning that users are overriding builtin plugins, otherwise people might forget to remove plugins that are now shipped by default from their projects.

packages/plugin-typescript/sources/index.ts Outdated Show resolved Hide resolved
@arcanis arcanis requested review from merceyz and paul-soporan March 24, 2022 17:41
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor details otherwise looks good. Any thoughts on #4253 (review)?

Comment on lines +29 to +33
const tsEnableAutoTypes = configuration.get(`tsEnableAutoTypes`)
?? xfs.existsSync(ppath.join(project.cwd, `tsconfig.json` as Filename));

if (!tsEnableAutoTypes)
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should check this when removing dependencies as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure I agree with this - while adding @types should only happen when the user uses TS, they can be safely removed even when the user doesn't, since @types/foo will always be related to foo.

packages/plugin-typescript/sources/index.ts Show resolved Hide resolved
@merceyz
Copy link
Member

merceyz commented Mar 24, 2022

The documentation (

berry/README.md

Lines 213 to 223 in 4ffd908

### Contrib plugins
Although developed on the same repository as Yarn itself, those plugins are optional and need to be explicitly installed through `yarn plugin import @yarnpkg/<plugin-name>`.
- [☆ plugin-constraints](packages/plugin-constraints) adds support for [constraints](https://yarnpkg.com/features/constraints) to Yarn.
- [☆ plugin-exec](packages/plugin-exec) adds support for using the [`exec:`](https://github.com/yarnpkg/berry/tree/master/packages/plugin-exec#documentation) protocol within your dependencies.
- [☆ plugin-interactive-tools](packages/plugin-interactive-tools) adds support for various interactive commands ([`yarn upgrade-interactive`](https://yarnpkg.com/cli/upgrade-interactive)).
- [☆ plugin-stage](packages/plugin-stage) adds support for the [`yarn stage`](https://yarnpkg.com/cli/stage) command.
- [☆ plugin-typescript](packages/plugin-typescript) improves the user experience when working with TypeScript.
- [☆ plugin-version](packages/plugin-version) adds support for the new [release workflow](https://yarnpkg.com/features/release-workflow).
- [☆ plugin-workspace-tools](packages/plugin-workspace-tools) adds support for the [`yarn workspaces foreach`](https://yarnpkg.com/cli/workspaces/foreach) command.
etc.) needs an update as well and the release script can be simplified a bit
yarn build:cli $(
yarn constraints query "
workspace(Cwd),
workspace_field(Cwd, 'scripts["update-local"]', _),
workspace_ident(Cwd, Ident),
sub_atom(Ident, 0, _, _, '@yarnpkg/plugin-')
" --json | jq -r '"--plugin='$(pwd)'/" + .Cwd' | xargs
)

@arcanis
Copy link
Member Author

arcanis commented Mar 25, 2022

Any thoughts on #4253 (review)?

I agree, but for now I'd prefer to land this PR without adding too much delay; I'll keep that in mind for a followup.

@arcanis arcanis requested a review from merceyz March 25, 2022 15:49
@@ -1,41 +1,6 @@
# This file contains the list of officially endorsed plugins.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem like we can update this file without breaking Yarn v3

const REMOTE_REGISTRY = `https://raw.githubusercontent.com/yarnpkg/berry/master/plugins.yml`;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed - at the very least I'll update the urls to point on the 3.2 tag rather than master.

@arcanis arcanis requested a review from merceyz March 28, 2022 18:18
@arcanis arcanis force-pushed the mael/all-the-plugins branch from e124d78 to ec9034e Compare March 29, 2022 10:20
@arcanis arcanis added the infra: debug container A debuggable container will spawn label Mar 29, 2022
@arcanis arcanis removed the infra: debug container A debuggable container will spawn label Mar 29, 2022
@arcanis arcanis merged commit 952e88e into master Mar 29, 2022
@arcanis arcanis deleted the mael/all-the-plugins branch March 29, 2022 15:19
@merceyz merceyz mentioned this pull request Apr 1, 2022
3 tasks
@merceyz merceyz mentioned this pull request Apr 12, 2022
3 tasks
@merceyz merceyz added this to the 4.0.0 milestone May 8, 2023
nigelzor added a commit to nigelzor/semantic-release-yarn that referenced this pull request Aug 3, 2023
in yarn 4, all first-party plugins are installed by default. trying to
install workspace-tools again fails.

ref: yarnpkg/berry#4253
hongaar pushed a commit to hongaar/semantic-release-yarn that referenced this pull request Sep 2, 2023
in yarn 4, all first-party plugins are installed by default. trying to
install workspace-tools again fails.

ref: yarnpkg/berry#4253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants