search

bmustiata / fast-live-reload

A live reload that works with all the possible browsers.
18 stars 5 forks source link

Fixing the regex match in update-notifier.js #3

Closed bbasic closed 8 years ago

bbasic commented 8 years ago

Previous regex would match a URL string with a hashtag considering it to be a page fragment, hence with an assumption that it can only occur as the last part of the URL. However, in single page applications we sometimes have a sign that acts as a delimiter between a base URL and routes leading to other locations of the single page application. In AngularJS for instance # or #! is used as the delimiter.

Fixing the regex to match a page fragment only for # that has no / sign anywhere after it. Any other hashtags found between slashes will be considered as part of the hostString.

Example of a valid URL in AngularJS application: http://somepage.io/base#/users/1/details In previous implementation this lead to an error in ParameterParser on evaluating hashParams.

bmustiata commented 8 years ago

Hi bbasic, Thank you for the find and for the fix.

Unfortunately the actual bug is in the ParameterParser, and not in the regexp. For example the RFC for URIs has this diagram explaining it:

         foo://example.com:8042/over/there?name=ferret#nose
         \_/   \______________/\_________/ \_________/ \__/
          |           |            |            |        |
       scheme     authority       path        query   fragment
          |   _____________________|__
         / \ /                        \
         urn:example:animal:ferret:nose

So the fragment is whatever comes after the sharp, and that is correctly handled by the regexp.

Having an if that checks if the match was done, might be something, but then even if the match is null using it anyway, that's priceless.

bmustiata commented 8 years ago

Since you're not committing anything that breaks things, would you be interested in getting commit rights + npm contributor?

I assume if other people are using it also for angular & co, it would make sense to get a fix before I have a chance to look over things.

bbasic commented 8 years ago

Hi, yes, I'm interested. Thanks for clarifying URI fragment.

bmustiata commented 8 years ago

I've added you as a collaborator. I need your npm account name, so you can also push updates.

bbasic commented 8 years ago

it's bbasic

bmustiata commented 8 years ago

Ok, done. On Oct 28, 2015 12:01 PM, "Becir Basic" notifications@github.com wrote:

it's bbasic

— Reply to this email directly or view it on GitHub https://github.com/bmustiata/fast-live-reload/pull/3#issuecomment-151802017 .