Skip to content

Conversation

@davilima6
Copy link

Addresses the proposed fix in #120 by providing a custom Node URL-based implementation for isUrl.

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If yes, please describe the breakage.

Please Describe Your Changes

@shellscape
Copy link
Owner

Thanks for taking the initiative here. I'm not keen on adding additional code to maintain. I know it's a simple function, but that means it needs tests and such as well. I'd much rather use a package that had its own tests that I didn't have to maintain.

@davilima6
Copy link
Author

davilima6 commented Aug 17, 2020

I think adding tests here could be worth the price of avoiding a situation like this from happening again. I think only the protocol-relative URLs condition branch needs to be tested. What do you think?

@davilima6
Copy link
Author

davilima6 commented Aug 17, 2020

Another issue is that CI fails because package depends on itself (on an outdated version, Idk if intentionally). I see it's imported for performance tests. Would you have any suggestion on how to sort out this one?

@davilima6 davilima6 mentioned this pull request Aug 17, 2020
8 tasks
@davilima6 davilima6 changed the title feat: remove dependency on is-url-superb Remove dependency on is-url-superb Aug 17, 2020
Davi Medeiros added 2 commits August 18, 2020 11:21
…plementation, with custom support for protocol-relative urls
This was used to allow imporing in performance tests.

This fork will not concern with those.
@davilima6 davilima6 marked this pull request as draft August 18, 2020 09:24
@davilima6
Copy link
Author

Moving PR to draft while we don't have an alternative for 089a196

@shellscape
Copy link
Owner

Closing in favor of #125. As noted on your other PR, I do sincerely and truthfully appreciate the work you put in on this.

@shellscape shellscape closed this Sep 16, 2020
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