Changes for Windows compatibility #2

jpbourgeon · 2017-04-15T09:32:42Z

Windows uses backslashes for OS paths and doesn't allow semicolons in folders and files names.

I introduced 2 changes in the router to handle these behaviours :

an optional paramChar to let the user customize the character used for parameterized routes. Semicolon is the default value. I moved the paramPattern definition inside the router accordingly, to apply the user's choice
a sanitization of the paths to prevent mixing of slashes and backslashes which messes the parameters's value indentification

Windows uses backslashes for OS paths and doesn't allow semicolons in folders and files names. I introduced 2 changes in the router to handle these behaviours : - an optional paramChar to let the user customized the character used for parameterized routes. Semicolon is the default value. I moved the paramPattern definition inside the router accordingly, to apply the user's choice - a sanitization of the paths to prevent mixing of slashes and backslashes which messes the parameters's value indentification

coveralls · 2017-04-15T09:34:03Z

Coverage remained the same at 100.0% when pulling fb2913f on jpbourgeon:patch-1 into 4492000 on jesseditson:master.

coveralls · 2017-04-15T09:34:03Z

Coverage remained the same at 100.0% when pulling fb2913f on jpbourgeon:patch-1 into 4492000 on jesseditson:master.

jesseditson · 2017-04-29T22:42:49Z

Thanks for the PR!

I'm on the fence as to if I want to allow the user to configure the param indicator. : is a pretty standard convention, and this lib (with your fixes for backslashes) would technically work if you specified module.exports.path in your parameterized routes.

I'd definitely accept a PR for dealing with the backslashes, but would at minimum like the custom param in a separate PR - and for what it's worth, I'm not quite sure I'd accept that PR.

I'm less familiar with non-unix FS, can you think of a sensible default to use instead of : on a fs that does not allow files to start with that char? I'd be much more inclined to just extend the param regex than make it configurable. Something like const paramPattern = /(?::|__)([^\/]+)/ (which would match __id.js the same as :id.js

jesseditson

Let's just do line 63 for now, and either remove paramChar or change the paramPattern regex to support a windows-compatible convention.

jpbourgeon · 2017-05-04T19:58:44Z

Hi

I understand that this PR looks like two changes instead of one. And at the same time, I thought carefully before submitting both at once.

: and other characters are not allowed at all on Windows systems. No Windows developer would be able to benefit from your package if we only fix the backslash limitation.

Compatibility on Windows platforms needs both the ability to handle backslashes AND the possibility to set a custom character to handle parameterized routes. The latter is important since there is no sensible default across platforms : on Windows each developer will define his own filenaming convention, and any choice we might take here would be arbitrary and might bother a team or another.

More generally, providing parameters empowers the developer to decide what suits best for his app on case per case basis. This is generally a good practice for libraries and APIs.

In my opinion, the whole PR doesn't change the philosophy or the behaviour of your package, it defines a sensible default (:) with a solid alternative, which makes your plugin cross-platform and more future proof.

Do you still want to stick with one half of the patch ?

RemyLespagnol · 2017-05-04T20:09:38Z

Hi @jpbourgeon,

I made PR for backslash just before : #5
I use replace instead of split + join. Maybe you see error with that ?

For the params folder, like I said, and like you said, we don't have convention on folder.
So if @jesseditson want to avoid configuration for his router, I said that we can use the % symbol.
But what you said @jpbourgeon ise very logical too.

I think, any developer will be happy with the two options. If we use this package, we need to respect the readme for that kinds of things.

jesseditson · 2017-05-04T20:18:13Z

Hey there!

: and other characters are not allowed at all on Windows systems. No Windows developer would be able to benefit from your package if we only fix the backslash limitation.

With #5 in, windows users could in fact use the lib by exporting path from any module that contains a :param, e.g.

// routes/whatever.js
module.exports.GET = async function(req, res) {
  send(res, 200, {})
}
// exposing a "path" will override the fs-generated one.
// This is nice if you wanted to avoid making a really deep tree for a one-off path (like for oauth callbacks)
// or if you just want to avoid putting `:` in your file/folder names or something
module.exports.path = '/foo/:bar'

(documented under custom path in the README).

However, I can understand the argument that this removes some of the simplicity and utility from the lib.

That same argument however is why I would also advocate for a non-configurable default. This has some advantages:

implementors all speak the same language. If I look at your repo using fs-router, I won't be confused as to which files are parameterized.
we retain the ethos of "use your fs as your router", which IMO is the point of this lib. There's edge case functionality, but the idea behind this lib is that it defines an API for the FS. I want this API to be deterministic, otherwise we increase the cognitive load for the consuming user.

Basically, the way I see it is that either we're forcing the user to make a difficult naming decision (what should define a parameterized route?) or we're making the decision for them, which IMO is much more in line with the spirit of this library. If a user needs a highly configurable router, nearly every other router for micro has all the options you could ever dream of.

More generally, providing parameters empowers the developer to decide what suits best for his app on case per case basis. This is generally a good practice for libraries and APIs.

I see your point, but this library is intentionally an "omakase" abstraction, it's main feature is that it removes config. Adding config to it IMO defeats it's purpose.

Backslashes are a no-brainer, and we've gotten them in via #5 - let's make this PR add a good default for windows (% seems good). We can take pride in that we've reduced a complex problem to a one-line regex change. /:([^\/]+)/ -> /:([^\/%]+)/

coveralls · 2017-05-04T20:24:06Z

Coverage remained the same at 100.0% when pulling 32822cf on jpbourgeon:patch-1 into a995cb4 on jesseditson:master.

RemyLespagnol · 2017-05-04T20:24:30Z

index.js

-// create global paramPattern but define it in the router to allow custom char for parameterized routes
-let paramPattern
+
+const paramPattern = /:([^\/%]+)/


I don't understand this regex.

We want :params or %params.
So regex become /(:|%)([^\/]+)/ ?

hehe you're right! I mistyped in my comment, your regex is correct. (should be /(:|%)([^/]+)/)

That's my bad, sorry @jpbourgeon!

Ahaha, I searched but i can't see why this regex !^^

Oups I copy/pasted it without reading it

AFAIK there is absolutely nothing wrong with both approach and for small to medium replace cases they are both equivalent. It is a matter of preference and replace is more aligned with the current style of the plugin. It would be faster for deep trees too

RemyLespagnol

Yeah ;)

jesseditson

Awesome, thank you so much for bearing with me, and thanks for the thoughtful discussion. Will push a patch with this when I get home!

jesseditson · 2017-05-04T20:33:15Z

Whoops, tests failed - will merge once those pass! I'll want tests for % prefix too but happy to add those myself after merge if it's a hassle to add them! Just let me know

RemyLespagnol · 2017-05-04T20:44:54Z

Ahah, it's because we made some mistakes !

It's /(:|%)([^\/]+)/ and not /(:|%)([^/]+)/

I don't know why /:([^/]+)/ works before.

RemyLespagnol · 2017-05-04T20:48:05Z

Hum ok it's not that... tests failed again.

But definively we need the backslash in the regex, don't it ?

jesseditson · 2017-05-04T20:52:05Z

The backslash was actually stripped here: https://github.com/jesseditson/fs-router/pull/3/files#diff-168726dbe96b3ce427e7fedce31bb0bcL5, it is not necessary since we're defining a regex literal.

Test results point to the regex not actually matching at all, I can troubleshoot later but I'd recommend running the tests locally and making sure they pass on windows (note to self, I need to add windows to the CI so we don't break there....)

jpbourgeon · 2017-05-04T21:03:13Z

I tested the regex on https://regex101.com/ and it seems valid to me. Indeed the \ is not mandatory.

We cannot run your tests on windows since some files and folders contain :.
Git refuses to check those files out on windows !

jesseditson · 2017-05-04T21:04:12Z

Ah, I see! I'll get some conclusive results later today then, and put some thought in to dealing with the file paths in a windows env.

jpbourgeon · 2017-05-04T21:05:39Z

👍

jesseditson · 2017-05-05T18:59:22Z

Checked this out last night but forgot to update here - the issue here is that the optional group is replacing the capture group, so the routes only match the : bit. The fix is to just make the optional group non-capturing - the paramPattern regex can be replaced with /(?::|%)([^\/]+)/ to fix this.

A different but less preferable fix IMO would be to use matches[2] in addMatch when collecting paramNames (hopefully that helps clarify the issue).

Also, I noticed we'll need to update the check for index files to include % on this line: https://github.com/jesseditson/fs-router/pull/2/files#diff-168726dbe96b3ce427e7fedce31bb0bcR21

and update the comment here: https://github.com/jesseditson/fs-router/pull/2/files#diff-168726dbe96b3ce427e7fedce31bb0bcR18 to reflect the new pattern.

coveralls · 2017-06-07T10:17:29Z

Coverage remained the same at 100.0% when pulling b3d53db on jpbourgeon:patch-1 into a995cb4 on jesseditson:master.

coveralls · 2017-06-07T10:17:29Z

Coverage remained the same at 100.0% when pulling b3d53db on jpbourgeon:patch-1 into a995cb4 on jesseditson:master.

jpbourgeon · 2017-06-07T10:17:50Z

Sorry for the delay @jesseditson, I thought that you would close this PR in favor of a more regular commit

jesseditson · 2017-06-08T05:16:20Z

Hey! No worries about delay, I wanted you to get credit for the contribution, and this isn't blocking anyone AFAIK.

Thanks for hanging with this issue, merging now!

jesseditson · 2017-06-08T05:31:19Z

Published in v0.4.0

jesseditson requested changes Apr 29, 2017

View reviewed changes

jesseditson mentioned this pull request May 4, 2017

Error on Windows #4

Closed

Merge branch 'master' into patch-1

32822cf

RemyLespagnol reviewed May 4, 2017

View reviewed changes

Are we good now ?

21d1373

RemyLespagnol approved these changes May 4, 2017

View reviewed changes

jesseditson approved these changes May 4, 2017

View reviewed changes

Ahhhrrr Backslashes!

dcfb5d0

jesseditson mentioned this pull request May 4, 2017

Add windows tests to CI #6

Open

Fixes from @jesseditson

b3d53db

jesseditson merged commit 3cd0e6a into jesseditson:master Jun 8, 2017

jpbourgeon deleted the patch-1 branch June 8, 2017 12:40

jesseditson mentioned this pull request Aug 31, 2017

Add info about windows to readme #7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for Windows compatibility #2

Changes for Windows compatibility #2

jpbourgeon commented Apr 15, 2017 •

edited

Loading

coveralls commented Apr 15, 2017

coveralls commented Apr 15, 2017

jesseditson commented Apr 29, 2017

jesseditson left a comment

jpbourgeon commented May 4, 2017

RemyLespagnol commented May 4, 2017

jesseditson commented May 4, 2017

coveralls commented May 4, 2017 •

edited

Loading

RemyLespagnol May 4, 2017

jesseditson May 4, 2017 •

edited

Loading

jesseditson May 4, 2017

RemyLespagnol May 4, 2017

jpbourgeon May 4, 2017

RemyLespagnol left a comment

jesseditson left a comment

jesseditson commented May 4, 2017

RemyLespagnol commented May 4, 2017 •

edited

Loading

RemyLespagnol commented May 4, 2017

jesseditson commented May 4, 2017

jpbourgeon commented May 4, 2017 •

edited

Loading

jesseditson commented May 4, 2017

jpbourgeon commented May 4, 2017

jesseditson commented May 5, 2017

coveralls commented Jun 7, 2017

coveralls commented Jun 7, 2017

jpbourgeon commented Jun 7, 2017

jesseditson commented Jun 8, 2017

jesseditson commented Jun 8, 2017

Changes for Windows compatibility #2

Changes for Windows compatibility #2

Conversation

jpbourgeon commented Apr 15, 2017 • edited Loading

coveralls commented Apr 15, 2017

coveralls commented Apr 15, 2017

jesseditson commented Apr 29, 2017

jesseditson left a comment

Choose a reason for hiding this comment

jpbourgeon commented May 4, 2017

RemyLespagnol commented May 4, 2017

jesseditson commented May 4, 2017

coveralls commented May 4, 2017 • edited Loading

RemyLespagnol May 4, 2017

Choose a reason for hiding this comment

jesseditson May 4, 2017 • edited Loading

Choose a reason for hiding this comment

jesseditson May 4, 2017

Choose a reason for hiding this comment

RemyLespagnol May 4, 2017

Choose a reason for hiding this comment

jpbourgeon May 4, 2017

Choose a reason for hiding this comment

Oups I copy/pasted it without reading it

RemyLespagnol left a comment

Choose a reason for hiding this comment

jesseditson left a comment

Choose a reason for hiding this comment

jesseditson commented May 4, 2017

RemyLespagnol commented May 4, 2017 • edited Loading

RemyLespagnol commented May 4, 2017

jesseditson commented May 4, 2017

jpbourgeon commented May 4, 2017 • edited Loading

jesseditson commented May 4, 2017

jpbourgeon commented May 4, 2017

jesseditson commented May 5, 2017

coveralls commented Jun 7, 2017

coveralls commented Jun 7, 2017

jpbourgeon commented Jun 7, 2017

jesseditson commented Jun 8, 2017

jesseditson commented Jun 8, 2017

jpbourgeon commented Apr 15, 2017 •

edited

Loading

coveralls commented May 4, 2017 •

edited

Loading

jesseditson May 4, 2017 •

edited

Loading

RemyLespagnol commented May 4, 2017 •

edited

Loading

jpbourgeon commented May 4, 2017 •

edited

Loading