search

deprecate / metal-uri

Class for parsing and formatting URIs.
Other
3 stars 7 forks source link

Use url-parse module instead of relying on native behavior that differs between environments #20

Closed robframpton closed 6 years ago

robframpton commented 6 years ago

Replaces native use of the URL constructor and parsing urls with anchor tags with a combination of the url-parse package and resolve-pathname package.

It also removes the node.js entry point so that server side use should function exactly the same as client side use.

In regards to the need for something like the resolve-pathname dependency, it's to ensure this behavior in particular doesn't break.

robframpton commented 6 years ago

If we're comfortable with this change, I'd like to remove the ability to set a custom parser. Now that we don't need the different parser for Node.js it doesn't seem that useful of a feature. Thoughts?

ipeychev commented 6 years ago

Hey Rob, I also thought about that. In general it is tempting to allow developer to change the parsing function. This might be useful in case of emergency - if for some reason our current one doesn't work, he will be able to replace it with something else. However, the probability for that to happen is very low:

  1. If the developer wants to overwrite the parsing function without forking metal-uri he will have to keep the new implementation in his project and pass it via setParseFn. I don't think he will be happy to do this.
  2. If he forks the module, then he would rather overwrite the content of parsing function, instead to create a new implementation and pass it via setParseFn.

Keeping that in mind, I would vote for removing it and releasing v3.0.0.

jbalsas commented 6 years ago

Hey @ipeychev, @eduardolundgren, I'm going to be merging this to the develop branch in preparation for a 3.0.0 release. That way we know for sure that this changes won't affect people in the 2.x branch that might be forced to update their builds.

@Robert-Frampton, can you take care of removing the setParseFn feature and sending a followup PR?

@vbence86, final double-check here before I merge, please? :)

vbence86 commented 6 years ago

LGTM. Nice piece of work.

robframpton commented 6 years ago

Hey @jbalsas, no problem 👍

robframpton commented 6 years ago

https://github.com/metal/metal-uri/pull/21