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

feat: when initializing the Configuration, auto fetch the missing plugins #4912

Merged
merged 23 commits into from
Oct 14, 2022

Conversation

jj811208
Copy link
Contributor

@jj811208 jj811208 commented Sep 27, 2022

What's the problem this PR addresses?

Closes #4464

When the user loses the .yarn/plugins directory, all yarn commands will fail.

...

How did you fix it?

1. I changed the loading order of the Configuration 0c3221d

reason

before:
1. load RC file core fields
2. load core and third-party plugins
3. load RC file remaining fields

after:
1. load RC file core fields
2. load core plugins
3. load RC file remaining fields
4. load third-party plugins

2. when initializing the Configuration, check if the plugin exists and try to fetch the missing plugins 1c5c3a0

3. test 90a8e97 ead886a

...

Additional attempts

jj811208#4

...

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.

@jj811208 jj811208 changed the title feat: make yarn install to fetch missing plugins feat: make yarn install also fetch missing plugins Sep 27, 2022
@jj811208 jj811208 marked this pull request as draft September 28, 2022 04:22
@jj811208 jj811208 force-pushed the feat/auto-install-missing-plugins branch from 95379ea to 15dccb1 Compare September 28, 2022 15:20
@jj811208 jj811208 marked this pull request as ready for review September 28, 2022 15:42
@arcanis
Copy link
Member

arcanis commented Sep 28, 2022

Thanks for tackling this! It's been a longstanding issue, I'd be nice to find a solution.

In terms of design, a couple of comments:

  • I'm not fond of branching depending on process.argv. That's already something we did in Classic, and it ended up being such a mess that we had to implement our own CLI framework (Clipanion); I'd prefer to avoid falling into this trap again!

  • The current dance of "Partially load the configuration" doesn't look ideal; I think it'd be best if we could guarantee that the Configuration class is always complete when running the command - that would avoid many potential footguns.

So what I'm thinking is, couldn't we rather fetch the missing plugins while instantiating the main configuration? Perhaps even transparently, to avoid messing with the --json output (unless an error occurs, like a network error). This way, we wouldn't need to add new options to Configuration.find and we would know that, once it resolves, the resulting config object is ready to be used by the commands.

@jj811208
Copy link
Contributor Author

jj811208 commented Sep 28, 2022

@arcanis

Thanks for tackling this! It's been a longstanding issue, I'd be nice to find a solution.

I'm really glad to hear that😀.

In terms of design, a couple of comments:

I'm not fond of branching depending on process.argv. [...]

You're right, I don't like this approach either, but after trying for a long time, I couldn't find a better direction, so I had to leave a long comment in my poor English😢.

[...] couldn't we rather fetch the missing plugins while instantiating the main configuration? [...]

This is my first attempt to try to solve this issue: 6062beb (in my fork)

This is similar to what you said, I think this code affects less scope and the user experience will be great because the user doesn't even need to run yarn install.

But I'm not sure if it's good semantics to fetch missing plugins over the network when initializing Configuration.

If you like this approach, I can refactor this PR in this direction😀.

@jj811208 jj811208 force-pushed the feat/auto-install-missing-plugins branch from 15dccb1 to 61681cd Compare September 29, 2022 10:02
@jj811208 jj811208 changed the title feat: make yarn install also fetch missing plugins feat: when initializing the Configuration, auto fetch the missing plugins Sep 29, 2022
@jj811208 jj811208 marked this pull request as draft September 29, 2022 10:20
@jj811208 jj811208 force-pushed the feat/auto-install-missing-plugins branch from 61681cd to c916600 Compare September 29, 2022 13:40
@merceyz merceyz self-requested a review September 29, 2022 13:44
@jj811208
Copy link
Contributor Author

jj811208 commented Sep 29, 2022

hi @arcanis

I tried to follow the direction you mentioned but found that my test was broken(self-signed certificate in certificate chain), I guess this is because httpsCaFilePath wasn't loaded yet and I was already using httpUtils.

This made me realize that when I fetch the missing plugins via "configuration", I haven't loaded any network and HTTP settings in yarnrc yet.

Do you think it's a good direction to load all yarnrc settings before loading all third-party plugins? I'm not sure if this will cause some third-party plugins to break😢.

@jj811208 jj811208 force-pushed the feat/auto-install-missing-plugins branch from c916600 to ffd7257 Compare September 29, 2022 14:23
@jj811208 jj811208 marked this pull request as ready for review September 29, 2022 14:55
@arcanis
Copy link
Member

arcanis commented Sep 29, 2022

Yes, ideally we should load the core settings before the plugin ones. That's important for the proxy, but also for enableNetwork.

@jj811208 jj811208 force-pushed the feat/auto-install-missing-plugins branch 2 times, most recently from 1c96c0d to a206c93 Compare September 29, 2022 19:18
@jj811208
Copy link
Contributor Author

jj811208 commented Sep 29, 2022

I change the loading order in this commit 0c3221d

before:

  1. load RC file core fields
  2. load core and third-party plugins
  3. load RC file remaining fields

after:

  1. load RC file core fields
  2. load core plugins
  3. load RC file remaining fields
  4. load third-party plugins

Now it works well 😀

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.

In order to do this we need to have a checksum of the plugin in .yarnrc.yml so we can ensure the plugin didn't change.

@jj811208
Copy link
Contributor Author

jj811208 commented Sep 30, 2022

After some thought, if we want the third-party plugins to have checksum too, I realized that yarn third-party plugins are almost a devDependency🤔.

Maybe we should manage third-party plugins as a dependency.

I have thought of some advantages and disadvantages

Advantages

Third Party Plugin Developers

  • Can use npm registry
    • Easy to do version control (currently they have to do it themselves via URL)
    • No need to find a static server to put the plugin on
    • Can look at the analysis data (e.g. Weekly Downloads)

Users

  • Reduce learning costs
    • We can reduce the number of commands (e.g. yarn plugin import)
    • We can stop setting up plugins with yarnrc (use package.dependency)
  • Easy to upgrade and downgrade plugins (currently only manual)

Yarn Teams

  • Plugins can also share pnp, cache, plugin-patch and other feature
  • This issue that this PR wants to solve will no longer be a problem
  • This means that yarn third-party plugins will have package.json, which adds a lot of possibilities

Disadvantages

  • I'm not sure if writing a core plugin can smoothly upgrade (feels like there's a high probability of breaking-change)
  • Perhaps the workload will be large
  • This PR is almost useless 😢

@arcanis
Copy link
Member

arcanis commented Sep 30, 2022

That'd be a fairly complex undertaking, since plugins need to be installed for packages to be fetched. I think we should remain simple.

@jj811208
Copy link
Contributor Author

OK, then I will try to add plugin checksum to the RC file

@jj811208 jj811208 force-pushed the feat/auto-install-missing-plugins branch 4 times, most recently from 2a3f021 to dc8d550 Compare September 30, 2022 15:45
@jj811208
Copy link
Contributor Author

jj811208 commented Sep 30, 2022

  • when execute the yarn plugin import command, add(update) the plugin checksum to the RC File
    e84fd78

  • when fetch the missing plugins during the initial configuration, add(update) the plugin checksum to the RC File
    bebc9db 09177cb

  • test - make sure that the RC File's plugin checksum is always up to date after each plugin update
    2a3f021

If there is anything missing, please feel free to talk to me!

@jj811208 jj811208 force-pushed the feat/auto-install-missing-plugins branch from dc8d550 to e5fc815 Compare September 30, 2022 16:02
@jj811208 jj811208 force-pushed the feat/auto-install-missing-plugins branch from 3ea2669 to e5fc815 Compare October 2, 2022 14:44
@jj811208 jj811208 force-pushed the feat/auto-install-missing-plugins branch from cdacf49 to 724ba5c Compare October 13, 2022 18:21
@arcanis arcanis merged commit f3775aa into yarnpkg:master Oct 14, 2022
@jj811208 jj811208 deleted the feat/auto-install-missing-plugins branch October 14, 2022 19:18
@landsman
Copy link

landsman commented Jul 8, 2023

Can you please help me find in which version was this feature released? 🙏
I'm lost there, sadly it's not mentioned in Releases, there is just the diff link...

@landsman
Copy link

landsman commented Jul 8, 2023

Can we please cherry-pick this to v3.x version? I can't wait to v4 release 😢

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

Successfully merging this pull request may close these issues.

[Feature] yarn install should install missing plugins
4 participants