Skip to content

refactor!: update hooks to accept objects #4524

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jdanil
Copy link
Contributor

@jdanil jdanil commented Jun 5, 2022

What's the problem this PR addresses?

Addresses one of the checklist items listed for Yarn 4 in #3591.

How did you fix it?

Updated all hooks and their interfaces to replace positional parameters with an object.

Updated all the hooks in 1815afd.
Removed unused attributes in 56703af.

Checklist

  • I have read the Contributing Guide.
  • 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.

Comment on lines 22 to 27
getNpmAuthenticationHeader?: ({ currentHeader, registry, configuration, ident }: {
currentHeader?: string,
registry: string,
configuration: Configuration,
ident?: Ident,
}) => Promise<string | undefined>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the 3rd parameter was already an object, I just merged the first two parameters into it rather than nesting them further.

Comment on lines 46 to 50
async downloadHosted(locator: Locator, options: FetchOptions) {
return options.project.configuration.reduceHook((hooks: Hooks) => {
return hooks.fetchHostedRepository;
}, null as FetchResult | null, locator, opts);
}, {current: null as FetchResult | null, locator, options});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed opt to options so that the interface is clearer for anyone using the hook.

@jdanil jdanil force-pushed the chore/hook-parameters branch from 8462bb2 to a0f5b80 Compare June 5, 2022 09:11
@merceyz merceyz changed the title chore!: update hooks to accept objects refactor!: update hooks to accept objects Jun 5, 2022
@merceyz merceyz mentioned this pull request Jun 5, 2022
13 tasks
@jdanil
Copy link
Contributor Author

jdanil commented Jun 6, 2022

I believe there is one remaining issue, and that is with reduceHook (in packages/yarnpkg-core/sources/Configuration.ts) which was previously using the first parameter of hooks as an initial value and passing each result into the next invocation of the hook. I haven't found a neat solution to handle this yet.

@jdanil jdanil force-pushed the chore/hook-parameters branch from c9d3e4c to 799130b Compare June 6, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants