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

buildAuthenticatedFetch no longer supports custom fetch as a first argument #3810

Open
1 task done
mrkvon opened this issue Dec 22, 2024 · 3 comments
Open
1 task done
Labels
bug Something isn't working

Comments

@mrkvon
Copy link

mrkvon commented Dec 22, 2024

This is either bug or (regressed) feature request.

Search terms you've used

buildAuthenticatedFetch

Impacted package

Which packages do you think might be impacted by the bug ?

  • solid-client-authn-core

Description

It looks like since v2, the library no longer supports passing custom fetch to buildAuthenticatedFetch (used to be first argument). That can be considered a breaking change and feature regression. Please note that others depend directly on buildAuthenticatedFetch, and passing custom fetch can be very useful. In my case building authenticated request in Cypress.

Expected result

It would be awesome if we can continue passing custom fetch to buildAuthenticatedFetch (e.g. as an option). I don't mind if API changes (e.g. passing fetch as optional property in second parameter).

@mrkvon mrkvon added the bug Something isn't working label Dec 22, 2024
@NSeydoux
Copy link
Contributor

NSeydoux commented Dec 23, 2024

Thanks for reporting this. It is indeed a braking change, although it wasn't documented as such because we don't document @inrupt/solid-client-authn-core as public API, it is used internally by our other libraries. It is good to know that this is a use case for community members, we will consider this for future changes and releases so that this doesn't happen again.

I can't commit to a timeline for this to be changed in a way that suits your use case, so in the interim would it be possible to inverse the responsibilities and build the customized fetch from the authenticated one? Something a bit like https://github.com/inrupt/solid-client-access-grants-js/blob/f074a59dd61b45bbaf4c6fc811ffd419626049ae/e2e/node/e2e.test.ts#L149, where you pass in the customizations and use the output of buildAuthenticatedFetch as a base?

If that doesn't work, I'm happy to assist making the change. Things were modified in 494af00, where you'll notice the unauthFetch parameter was removed. If you add it back into the options, it will be a non-breaking change (from a library standpoint) that will make it possible to fix the regression with a minimal code change on your end. What do you think?

@mrkvon
Copy link
Author

mrkvon commented Dec 23, 2024

FYI CSS guide Automating authentication with Client Credentials depends on buildAuthenticatedFetch. It's the reason why I use it as well, both for sending authenticated requests in cypress (testing) and css-authn library.

@mrkvon
Copy link
Author

mrkvon commented Dec 23, 2024

Thank you for the elaborate reply! 🙏🏼

My use case here is to send authenticated requests during testing to CSS with cy.request, which is copied from the ingenious work by folks of SolidCryptPad. I haven't found a way to wrap cy.request around standard fetch, and I'm not sure it's possible.

For now, I have copied and edited the relevant file to the affected codebase. So, this is not urgent.

It would be indeed helpful if breaking changes to important core API get documented in the future. OTOH if fetch becomes a second-argument options' optional parameter, it won't have to be a breaking change. 🙂

Thank you for this amazing library! 🤩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants