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

Url module #4177

Merged
merged 9 commits into from
Jan 13, 2025
Merged

Url module #4177

merged 9 commits into from
Jan 13, 2025

Conversation

KhraksMamtsov
Copy link
Contributor

Type

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Url module has been introduced:

  • immutable setters with dual-function api
  • integration with UrlParams

Related

  • Related Issue #
  • Closes #

Copy link

changeset-bot bot commented Dec 20, 2024

🦋 Changeset detected

Latest commit: a517364

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 29 packages
Name Type
@effect/platform Patch
@effect/cli Patch
@effect/cluster-node Patch
@effect/cluster-workflow Patch
@effect/cluster Patch
@effect/experimental Patch
@effect/platform-browser Patch
@effect/platform-bun Patch
@effect/platform-node-shared Patch
@effect/platform-node Patch
@effect/rpc-http Patch
@effect/rpc Patch
@effect/sql-clickhouse Patch
@effect/sql-d1 Patch
@effect/sql-libsql Patch
@effect/sql-mssql Patch
@effect/sql-mysql2 Patch
@effect/sql-pg Patch
@effect/sql-sqlite-bun Patch
@effect/sql-sqlite-node Patch
@effect/sql Patch
@effect/ai Patch
@effect/ai-openai Patch
@effect/sql-sqlite-do Patch
@effect/sql-sqlite-react-native Patch
@effect/sql-sqlite-wasm Patch
@effect/cluster-browser Patch
@effect/sql-drizzle Patch
@effect/sql-kysely Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

packages/platform/src/Url.ts Outdated Show resolved Hide resolved
packages/platform/src/Url.ts Outdated Show resolved Hide resolved
packages/platform/src/Url.ts Show resolved Hide resolved
@KhraksMamtsov
Copy link
Contributor Author

@tim-smart done

* @category constructors
*/
export const make: {
(url: string | URL, base?: string | URL | undefined): Either.Either<URL, Cause.IllegalArgumentException>
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi! Wouldn't it be great to have a limit on what parameters this function accepts? I'm talking about the situation when we pass URL objects as both the first and the second parameters. I think it's a good idea to check at compile time that the first parameter url cannot be of type URL, when the second base is passed, using for example function overloads.
When you attempt to pass 2 URL objects, the second one just gets ignored. It seems like the first parameter can even be a string representation of the URL and as long as the first parameter can be successfully parsed to the URL object without errors, it will completely dominate over the second parameter. Because of that, it's safe to assume that passing 2 URL objects to the URL's constructor is most likely a mistake and it's best to point the developer to it by typescript.

> const githubURL = new URL('http://github.com/nikelborm?param1=123')
undefined
> const googleURL = new URL('http://google.com/pizza?param2=qwe')
undefined
> const someURL = new URL(githubURL, googleURL)
undefined
> someURL
URL {
  href: 'http://github.com/nikelborm?param1=123',
  origin: 'http://github.com',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'github.com',
  hostname: 'github.com',
  port: '',
  pathname: '/nikelborm',
  search: '?param1=123',
  searchParams: URLSearchParams { 'param1' => '123' },
  hash: ''
}
> const someURL2 = new URL('http://github.com/nikelborm?param1=123', 'http://google.com/pizza?param2=qwe')
undefined
> someURL2
URL {
  href: 'http://github.com/nikelborm?param1=123',
  origin: 'http://github.com',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'github.com',
  hostname: 'github.com',
  port: '',
  pathname: '/nikelborm',
  search: '?param1=123',
  searchParams: URLSearchParams { 'param1' => '123' },
  hash: ''
}
> new URL('github.com/nikelborm?param1=123', 'http://google.com/pizza?param2=qwe')
URL {
  href: 'http://google.com/github.com/nikelborm?param1=123',
  origin: 'http://google.com',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'google.com',
  hostname: 'google.com',
  port: '',
  pathname: '/github.com/nikelborm',
  search: '?param1=123',
  searchParams: URLSearchParams { 'param1' => '123' },
  hash: ''
}
> new URL('github.com/nikelborm?param1=123')
Uncaught TypeError: Invalid URL
    at new URL (node:internal/url:818:25) {
  code: 'ERR_INVALID_URL',
  input: 'github.com/nikelborm?param1=123'
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you)
I removed URL from make (because it seems like a rather useless case, in my opinion - because making make(MyURL) looks like clone) and renamed it to fromString.
I also added a clone function.

@tim-smart tim-smart merged commit 8cd7319 into Effect-TS:main Jan 13, 2025
12 checks passed
@github-actions github-actions bot mentioned this pull request Jan 13, 2025
@nikelborm
Copy link
Contributor

yay 🎉

@nikelborm
Copy link
Contributor

BTW, shouldn't it be somehow programmatically connected to Schema.URL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants