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(pwned): use pathe.join to create haveIBeenPwned URL #240

Closed
wants to merge 1 commit into from

Conversation

DamianGlowala
Copy link

@DamianGlowala DamianGlowala commented Sep 20, 2023

This improves DX by removing a need for a user to remember to add a trailing slash at the end of the custom URL used by haveIBeenPwned.ts:

const matcherPwned = matcherPwnedFactory(fetch, zxcvbnOptions, {
-  url: "/api/pwnedpasswords/"
+  url: "/api/pwnedpasswords"
});

@DamianGlowala DamianGlowala changed the title refactor: use pathe.join to create haveIBeenPwned URL refactor(pwned): use pathe.join to create haveIBeenPwned URL Sep 20, 2023
@MrWook
Copy link
Collaborator

MrWook commented Sep 20, 2023

Hey thank you for you contribution.
I hesitation to include an entire library just to append a trailing slash to an URL when a simple if statement can accomplish the task. For example:

if (!url.endsWith('/')) {
  url = `${url}/`;
}

While libraries like pathe are excellent for file system operations, they're not necessary in this context, as we're only making HTTP requests using fetch.

Furthermore, it's important to consider that the URL might be customized by users who want to implement their own version of the pwned API. For example, the URL could be something like https://www.my-custom-domain.com/pwned?range=, which makes the trailing / less universally applicable.

So i think that i will not merge this Feature 🤔

@DamianGlowala
Copy link
Author

Thank you for your explanation. I agree with the points you raised and I'm happy to close this PR.

Instead, what would you say about being able to provide a custom fetching function for most flexibility? For instace, Nuxt provides $fetch, which I'd love to use in the following way instead of e.g. native fetch provided by the Node.js runtime. This allows for full freedom in combining custom URL with a range param:

const matcherFactory = matcherPwnedFactory(
  (range) => $fetch(`http://localhost:3000/api/pwnedpasswords/${range}`),
  ...
)

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.

2 participants