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(std): Add std/url module. #3527

Merged
merged 7 commits into from
Sep 1, 2023
Merged

feat(std): Add std/url module. #3527

merged 7 commits into from
Sep 1, 2023

Conversation

tr1ckydev
Copy link
Contributor

@tr1ckydev tr1ckydev commented Aug 8, 2023

Utilities for working with URL paths (#3515).

closes #3515

Utilities for working with URL paths.
@CLAassistant
Copy link

CLAassistant commented Aug 8, 2023

CLA assistant check
All committers have signed the CLA.

url/extname_test.ts Show resolved Hide resolved
Comment on lines +3 to +26
import { assertEquals } from "../assert/mod.ts";
import * as url from "./mod.ts";

const TESTSUITE: [[string | URL, ...string[]], URL][] = [
[
["https://deno.land", "std", "assert", "mod.ts"],
new URL("https://deno.land/std/assert/mod.ts"),
],
[
[new URL("https://deno.land"), "std", "assert", "mod.ts"],
new URL("https://deno.land/std/assert/mod.ts"),
],
[
[new URL("https://deno.land//"), "/std/", "/assert/", "//mod.ts"],
new URL("https://deno.land/std/assert/mod.ts"),
],
[["https://deno.land/"], new URL("https://deno.land/")],
];

Deno.test("join", function () {
for (const [[test_url, ...paths], expected] of TESTSUITE) {
assertEquals(url.join(test_url, ...paths), expected);
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I'm looking at the tests, I'm not sure if join even makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is it so?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it handles edgecases around double / so maybe it's still useful. Not entirely sure what the utility is. Thoughts @marvinhagemeister?

url/normalize.ts Outdated Show resolved Hide resolved
@tr1ckydev
Copy link
Contributor Author

Should common from std/path be implemented for url? Need suggestion.

@lino-levan
Copy link
Contributor

I'm trying to think of a usecase and can't come up with one off of the top of my head. I think a lot of these things can be done as a follow-up PR. So no pressure to get everything in right now!

@tr1ckydev
Copy link
Contributor Author

I have fixed couple of bugs and added new edge cases tests. Will be adding them later by today.

@timreichen
Copy link
Contributor

What would you think of adding a isUrl(input: string) function, either in this PR or the future?
Seems like both isurl and is-url have millions of weekly downloads.

@0f-0b
Copy link
Contributor

0f-0b commented Aug 9, 2023

What would you think of adding a isUrl(input: string) function, either in this PR or the future?

There is URL.canParse, although browser support is currently poor.

@tr1ckydev
Copy link
Contributor Author

https://github.com/tr1ckydev/pathurl
This is my package with all the proposed path functions for URLs along with various tests. If it's acceptable then I will port all the functions from here into std/url.

url/normalize.ts Outdated Show resolved Hide resolved
@kt3k
Copy link
Member

kt3k commented Sep 1, 2023

dirname might sound strange because it returns a URL instead of a string (it doesn't feel like 'name' to me), but this looks good enough for first pass.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@tr1ckydev LGTM. Nice work!

@kt3k kt3k merged commit 4e44fa7 into denoland:main Sep 1, 2023
8 checks passed
@tr1ckydev
Copy link
Contributor Author

dirname might sound strange because it returns a URL instead of a string (it doesn't feel like 'name' to me), but this looks good enough for first pass.

Agree, but as it's from url module so it can be expected. Even though dirname from path module returns parent path so works in case of url too imo.

@tr1ckydev
Copy link
Contributor Author

@tr1ckydev LGTM. Nice work!

Thank you. I have also send another pull request which bug fixes many functions.

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.

Add std/url with URL utilities
6 participants