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

refactor!: update hooks to accept objects #4524

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.

@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.

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.

None yet

2 participants