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(dependencies): remove urijs dependency #75

Merged
merged 2 commits into from
Apr 28, 2020
Merged

refactor(dependencies): remove urijs dependency #75

merged 2 commits into from
Apr 28, 2020

Conversation

panva
Copy link
Contributor

@panva panva commented Apr 14, 2020

Removes the (imho unnecessary) dependency on userland urijs module. I wanted to use the WHATWG URL API but alas that one does not support relative URLs.

@niftylettuce niftylettuce self-requested a review April 14, 2020 17:06
@panva
Copy link
Contributor Author

panva commented Apr 14, 2020

I am aware that the used methods are marked as deprecated but it's worth noting it's a documentation only deprecation that causes no deprecation warnings and that the functions are imho unlikely to be removed unless an alternative solution in core stdlib has that functionality, see here, and here.

Copy link
Member

@3imed-jaberi 3imed-jaberi left a comment

Choose a reason for hiding this comment

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

LGTM 👍 !
But, I have a suggestion ..

    replaced = parseUrl(replaced)
    if (typeof options.query === 'string') {
      replaced.search = options.query;
    } else {
      replaced.search = undefined
      replaced.query = options.query
    }
    
    return formatUrl(replaced)

@panva
Copy link
Contributor Author

panva commented Apr 16, 2020

LGTM 👍 !
But, I have a suggestion ..

    replaced = parseUrl(replaced)
    if (typeof options.query === 'string') {
      replaced.search = options.query;
    } else {
      replaced.search = undefined
      replaced.query = options.query
    }
    
    return formatUrl(replaced)

resolved, dunno what i was thinking ...

@niftylettuce niftylettuce merged commit 5660b68 into koajs:master Apr 28, 2020
@niftylettuce
Copy link
Contributor

I fixed the conflict in the wrong way, fixing, one moment.

niftylettuce added a commit that referenced this pull request Apr 28, 2020
@niftylettuce
Copy link
Contributor

Fixed 6131069

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.

3 participants