krakenjs / kraken-devtools

Development-time tools for kraken.js applications.
Other
40 stars 32 forks source link

Fix trailing slash after extension bug #50

Closed adurrive closed 9 years ago

adurrive commented 9 years ago

Fix the "Cannot read property '1' of null" exception when a trailing slash is used after an extension. Follow up of https://github.com/krakenjs/kraken-devtools/issues/24 and https://github.com/krakenjs/kraken-devtools/pull/27

Steps to reproduce:

In lib/middleware.js:

The issue comes from ext which discards any number of trailing / after a detected extension (.jpg and .jpg/ are both extensions in its view). These discarded / should be reintegrated in regex to allow proper matching. The fix shouldn't have any side effect on the extracted string, even when ext is empty.

This behavior comes from the implementation of path.extname in Node which can be tracked down to this regular expression splitPathRe = /^(\/?|)([\s\S]*?)((?:\.{1,2}|[^\/]+?|)(\.[^.\/]*|))(?:[\/]*)$/; (https://github.com/joyent/node/blob/master/lib/path.js)

Update: escape ext properly too (used Mozilla's escapeRegExp expression)

jasisk commented 9 years ago

Hey @adurrive.

Thanks for the PR. We have an (embarrassingly) long-standing PR (#38) to address this same issue as well as adding support for implicit index files. Having said that, the other PR doesn't take into account escaping the ext like yours does.

I'll pick your commit out of this PR to pull the ext escaping patch out, and apply it to the other PR to save you having to reauthor (while still getting your commit in). How does that sound?

adurrive commented 9 years ago

Sure, this was a quick bug fix updated from https://github.com/krakenjs/kraken-devtools/pull/27 but I actually find your new behavior on extensions much better!

jasisk commented 9 years ago

Cool. In that case, I'm gonna close this one out. Picked your commit into that PR (e2fd396c6c6f8517c715a6708b55586013ba1826). Thanks again!