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

Clean up url normalization #3515

Merged
merged 2 commits into from
Nov 3, 2016
Merged

Clean up url normalization #3515

merged 2 commits into from
Nov 3, 2016

Conversation

mourner
Copy link
Member

@mourner mourner commented Nov 2, 2016

Partially reverts #2702 while retaining the fix, getting rid of Node URL.parse and getting back to regexps and string manipulation. Reasons:

  • 40% less code
  • the build is 3.6% lighter (gzipped) without url module
  • it's 20x faster (normalizeTileURL on a typical satellite style profiling session would take ~60ms, now 2.5ms)
  • consistency and readability (previously we mixed both regexps and URL parsing together)
  • it feels hacky to use URL parsing on strings that are not really true urls

@lucaswoj @xrwang

@jfirebaugh
Copy link
Contributor

Can we use URL instead? I'd really like to avoid parsing with regexes.

@mourner
Copy link
Member Author

mourner commented Nov 2, 2016

@jfirebaugh that's what I started with first, but it looks like using it as a constructor for parsing URLs is not supported in IE11. I tried doing a fallback with the document.createElement('a') hack, but then ugly differences surfaced, like parsing mapbox://styles/foo as hostname="",path="//styles/foo" rather than hostname="styles",path="foo".

I'm convinced regexps are the cleanest and most robust solution here. Keep in mind that this code doesn't really normalize usual urls (it returns them as is), only Mapbox-style special strings that look like URLs but with subtle differences that are not handled by URL parsing libs (like hostname containing commas).

@jfirebaugh
Copy link
Contributor

MDN says:

When using a user agent where no constructor has been implemented yet, it is possible to access such an object using the Window.URL properties (prefixed with Webkit-based browser as Window.webkitURL).

But I tested in IE 11 and this statement is either misleading or untrue; there doesn't seem to be any way to construct a URL instance in IE 11. Too bad.

This is an aside, but we shouldn't think of the first component as a "hostname" -- in URI parlance, it's an "authority". It's true that tilesets are an ugly exception; at some point we need to change them to mapbox://tilesets/....

I'm not strongly opposed to using regexes, I just think the code would be clearer and more maintainable if it used a URL/URI parser. The equivalent native issue is mapbox/mapbox-gl-native#6183.

@lucaswoj
Copy link
Contributor

lucaswoj commented Nov 2, 2016

at some point we need to change them to mapbox://tilesets/....

We should do this sooner rather than later.

@mourner
Copy link
Member Author

mourner commented Nov 2, 2016

I'm not strongly opposed to using regexes, I just think the code would be clearer and more maintainable if it used a URL/URI parser.

I hoped for that too, but in practice found the PR approach much easier to read/update code in than the old one. The diff is hard to read, but try comparing the two sources (before/after) side by side.

@mourner
Copy link
Member Author

mourner commented Nov 3, 2016

I'm now trying out a middle-ground approach of writing a very tiny Mapbox url parsing utility, as suggested by @lucaswoj.

@mourner
Copy link
Member Author

mourner commented Nov 3, 2016

@jfirebaugh @lucaswoj can you review again after the last commit please?

Copy link
Contributor

@lucaswoj lucaswoj left a comment

Choose a reason for hiding this comment

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

I like this compromise.

  • As long as we aren't conforming to the URL spec, we shouldn't try to use the URL library.
  • parseURL encapsulates the regex parsing nicely.
  • There's nothing Hard about parsing mapbox:// URLs using a regex

🚢

@mourner mourner merged commit 44727a7 into master Nov 3, 2016
@mourner mourner deleted the cleanup-urls branch November 3, 2016 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants