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

Parameterized routing with filesystem for #91 #147

Closed
wants to merge 9 commits into
base: master
from

Conversation

@nodegin
Contributor

nodegin commented Oct 29, 2016

Hello, here is a proof of concept pull request for issue #91 that achieves parameterized routing using only filesystem structure.

Filesystem

2016-10-29 15 00 59

Code

import React from 'react'

export default class extends React.Component {
  static async getInitialProps ({ req, params }) {
    // params property must matches filename
    return { test: params.id }
  }

  render () {
    return (
      <b>{ this.props.test }</b>
    )
  }
}

Result

2016-10-29 14 58 14
2016-10-29 14 58 02
2016-10-29 14 57 48
2016-10-29 14 58 25

@CompuIves

This comment has been minimized.

Show comment
Hide comment
@CompuIves

CompuIves Oct 30, 2016

Contributor

Interesting PR! I like the idea very much, although I do have a concern that we're now deviating a bit from the way how nested routing was done in Next (exclusively using folders). I personally still prefer the idea with files like _id.js (or in this case @id.js, which seems better because it's more explicit) as initially proposed in #91.

Still, I really like the fact that we now have a working version of parameters in a file system! 👍

Contributor

CompuIves commented Oct 30, 2016

Interesting PR! I like the idea very much, although I do have a concern that we're now deviating a bit from the way how nested routing was done in Next (exclusively using folders). I personally still prefer the idea with files like _id.js (or in this case @id.js, which seems better because it's more explicit) as initially proposed in #91.

Still, I really like the fact that we now have a working version of parameters in a file system! 👍

@richardnias

This comment has been minimized.

Show comment
Hide comment
@richardnias

richardnias Oct 31, 2016

I agree with @CompuIves, something that uses nested directories feels better. I was working on something similar to this, which had files called :id.js, which would be placed in a users folder, rather than files like users_:id.js or users_@id.js. It was a really simple implementation (when resolving an URL to a filepath, as well as /my/path/index.js and /my/path.js, look for /my/:id.js). I haven't submitted a PR because it had shortcomings:

  • the param had to be :id.js (easily solved to be sure)
  • no support for routes like /users/:userId/posts/:postId

But both of these could be relatively easy to overcome - probably by taking a more advanced approach to changing the resolver than what I did.

richardnias commented Oct 31, 2016

I agree with @CompuIves, something that uses nested directories feels better. I was working on something similar to this, which had files called :id.js, which would be placed in a users folder, rather than files like users_:id.js or users_@id.js. It was a really simple implementation (when resolving an URL to a filepath, as well as /my/path/index.js and /my/path.js, look for /my/:id.js). I haven't submitted a PR because it had shortcomings:

  • the param had to be :id.js (easily solved to be sure)
  • no support for routes like /users/:userId/posts/:postId

But both of these could be relatively easy to overcome - probably by taking a more advanced approach to changing the resolver than what I did.

@nodegin

This comment has been minimized.

Show comment
Hide comment
@nodegin

nodegin Oct 31, 2016

Contributor

@nias-r I have tested with this PR which supports /users/:userId/posts/:postId, just name the filename like users_@uid_@action_@pid where params.action would be posts.

Contributor

nodegin commented Oct 31, 2016

@nias-r I have tested with this PR which supports /users/:userId/posts/:postId, just name the filename like users_@uid_@action_@pid where params.action would be posts.

@timoxley

This comment has been minimized.

Show comment
Hide comment
@timoxley

timoxley Nov 1, 2016

Contributor

I'd suggest keeping the mapping between files and routes as straightforward and non-magical as possible.

@nodegin is there a reason for using @ over :?

This means no users_@id.js -> users/:id magic, just keeping a strict mapping of routes to files/directories, via node/webpack's default module resolution algorithm.

i.e.
/users/:userId -> pages/users/:userId.js or pages/users/:userId/index.js

The second form with the pages/users/:userId directory + index.js would be useful in cases where you're nesting beyond a :param

> tree ./pages
./pages
├── index.js
└── users
    └── :userId
        ├── index.js
        └── posts
            └── :postId.js
Contributor

timoxley commented Nov 1, 2016

I'd suggest keeping the mapping between files and routes as straightforward and non-magical as possible.

@nodegin is there a reason for using @ over :?

This means no users_@id.js -> users/:id magic, just keeping a strict mapping of routes to files/directories, via node/webpack's default module resolution algorithm.

i.e.
/users/:userId -> pages/users/:userId.js or pages/users/:userId/index.js

The second form with the pages/users/:userId directory + index.js would be useful in cases where you're nesting beyond a :param

> tree ./pages
./pages
├── index.js
└── users
    └── :userId
        ├── index.js
        └── posts
            └── :postId.js
@nodegin

This comment has been minimized.

Show comment
Hide comment
@nodegin

nodegin Nov 1, 2016

Contributor

@timoxley someone point out that : might be not work properly on some systems.

Contributor

nodegin commented Nov 1, 2016

@timoxley someone point out that : might be not work properly on some systems.

@jharmn

This comment has been minimized.

Show comment
Hide comment
@jharmn

jharmn Nov 1, 2016

There is a nice implementation of directory-style routing, similar to @timoxley 's suggestion, which I've used for APIs in swaggerize-express (see Path Routing), originally written by @tlivings:

  |--users
  |    |--{id}.js
  |    |--{id}
  |        |--foo.js

Note the use of {id} for parameters, which is a much more common convention (and avoids the : issue).
In the original PR, the mapping of @ and _ to params and directories seems a little arbitrary and easy to confuse.

jharmn commented Nov 1, 2016

There is a nice implementation of directory-style routing, similar to @timoxley 's suggestion, which I've used for APIs in swaggerize-express (see Path Routing), originally written by @tlivings:

  |--users
  |    |--{id}.js
  |    |--{id}
  |        |--foo.js

Note the use of {id} for parameters, which is a much more common convention (and avoids the : issue).
In the original PR, the mapping of @ and _ to params and directories seems a little arbitrary and easy to confuse.

@giacomorebonato

This comment has been minimized.

Show comment
Hide comment
@giacomorebonato

giacomorebonato Nov 1, 2016

Contributor

I see a problem in naming files like {id}.js when you start to have a lot of file with the same name in a project and it is not comfortable anymore to "jump" to the one you want.

Contributor

giacomorebonato commented Nov 1, 2016

I see a problem in naming files like {id}.js when you start to have a lot of file with the same name in a project and it is not comfortable anymore to "jump" to the one you want.

@timoxley

This comment has been minimized.

Show comment
Hide comment
@timoxley

timoxley Nov 1, 2016

Contributor

@giacomorebonato nobody stopping you adding context e.g. {userId}.js

Contributor

timoxley commented Nov 1, 2016

@giacomorebonato nobody stopping you adding context e.g. {userId}.js

@CompuIves

This comment has been minimized.

Show comment
Hide comment
@CompuIves

CompuIves Nov 1, 2016

Contributor

I really like the idea of using {} instead of _. Much more explicit. Perfect scenario would be to do the initial proposal of #91 (like explained by @jharmn) but with {} instead of _.

Do you want to implement this @nodegin? I can do this as well if you're busy.

Contributor

CompuIves commented Nov 1, 2016

I really like the idea of using {} instead of _. Much more explicit. Perfect scenario would be to do the initial proposal of #91 (like explained by @jharmn) but with {} instead of _.

Do you want to implement this @nodegin? I can do this as well if you're busy.

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Nov 1, 2016

This looks great! But I agree with @CompuIves. I might suggest an alternate of ${} as this fits the js pattern of vars in strings as of es6

mattapperson commented Nov 1, 2016

This looks great! But I agree with @CompuIves. I might suggest an alternate of ${} as this fits the js pattern of vars in strings as of es6

@tlivings

This comment has been minimized.

Show comment
Hide comment
@tlivings

tlivings Nov 1, 2016

@mattapperson the purpose of {someParam} is for consistency with swagger path definitions (not to mention hapi).

There's nothing wrong with ${} but why bother matching es6 template literals or having an additional character?

tlivings commented Nov 1, 2016

@mattapperson the purpose of {someParam} is for consistency with swagger path definitions (not to mention hapi).

There's nothing wrong with ${} but why bother matching es6 template literals or having an additional character?

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Nov 1, 2016

@tlivings I dont disagree with you at all, but this project hides hapi, and it's not swagger. It is a server side react server. By it's nature it is JS first. the most JS way to do this in my mind is to use Template literals style.
That said I have NOTHING against {someParam}, ${someParam} just feels a little more pure to me.

mattapperson commented Nov 1, 2016

@tlivings I dont disagree with you at all, but this project hides hapi, and it's not swagger. It is a server side react server. By it's nature it is JS first. the most JS way to do this in my mind is to use Template literals style.
That said I have NOTHING against {someParam}, ${someParam} just feels a little more pure to me.

@tlivings

This comment has been minimized.

Show comment
Hide comment
@tlivings

tlivings Nov 1, 2016

No, I get it — I just referenced the other projects because that may also be what people are more familiar with.

tlivings commented Nov 1, 2016

No, I get it — I just referenced the other projects because that may also be what people are more familiar with.

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Nov 1, 2016

@tlivings I feel ya! Another counter point to my argument though would also be variables in JSX are {param name} so....

mattapperson commented Nov 1, 2016

@tlivings I feel ya! Another counter point to my argument though would also be variables in JSX are {param name} so....

@dstreet

This comment has been minimized.

Show comment
Hide comment
@dstreet

dstreet Nov 1, 2016

Contributor

Does anyone know of an existing system using a filesystem api like this?

Contributor

dstreet commented Nov 1, 2016

Does anyone know of an existing system using a filesystem api like this?

@timoxley

This comment has been minimized.

Show comment
Hide comment
@timoxley

timoxley Nov 1, 2016

Contributor

@tlivings @mattapperson template literal syntax is a non-starter due to shell variable expansion.

e.g. try creating a file named ${id}.js or ${USER}.js in your terminal.

Contributor

timoxley commented Nov 1, 2016

@tlivings @mattapperson template literal syntax is a non-starter due to shell variable expansion.

e.g. try creating a file named ${id}.js or ${USER}.js in your terminal.

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson commented Nov 1, 2016

Hah good point @timoxley!

@timoxley

This comment has been minimized.

Show comment
Hide comment
@timoxley

timoxley Nov 1, 2016

Contributor

@dstreet see above comment about swaggerize-express.

Contributor

timoxley commented Nov 1, 2016

@dstreet see above comment about swaggerize-express.

@dstreet

This comment has been minimized.

Show comment
Hide comment
@dstreet

dstreet Nov 1, 2016

Contributor

@timoxley Thanks. Not sure how I missed that

Contributor

dstreet commented Nov 1, 2016

@timoxley Thanks. Not sure how I missed that

@nodegin

This comment has been minimized.

Show comment
Hide comment
@nodegin

nodegin Nov 1, 2016

Contributor

Hi guys, I have updated this PR to use the format as @jharmn suggested:

  |--users
  |    |--{id}.js
  |    |--{id}
  |        |--foo.js

2016-11-02 04 49 50
2016-11-02 04 50 18
2016-11-02 04 50 10

But I found a bug that causing HMR function to be broken when the link is in /user/{id}/foo format, but not /user/{id}. Help needed for investigating..

Contributor

nodegin commented Nov 1, 2016

Hi guys, I have updated this PR to use the format as @jharmn suggested:

  |--users
  |    |--{id}.js
  |    |--{id}
  |        |--foo.js

2016-11-02 04 49 50
2016-11-02 04 50 18
2016-11-02 04 50 10

But I found a bug that causing HMR function to be broken when the link is in /user/{id}/foo format, but not /user/{id}. Help needed for investigating..

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Nov 4, 2016

@nodegin I suggest/request this gets merged and a new ticket get created for the known HMR bug. This is a major feature that allows next.js to be used in much larger apps. The HMR bug is annoying but not critical. This feature however is critical to many apps (like mine, selfishly)

mattapperson commented Nov 4, 2016

@nodegin I suggest/request this gets merged and a new ticket get created for the known HMR bug. This is a major feature that allows next.js to be used in much larger apps. The HMR bug is annoying but not critical. This feature however is critical to many apps (like mine, selfishly)

@nodegin

This comment has been minimized.

Show comment
Hide comment
@nodegin

nodegin Nov 5, 2016

Contributor

@mattapperson Only collaborators can merge this PR

Contributor

nodegin commented Nov 5, 2016

@mattapperson Only collaborators can merge this PR

@mikecardwell

This comment has been minimized.

Show comment
Hide comment
@mikecardwell

mikecardwell Nov 5, 2016

Using this branch, I created a "pages/index.js". Requests to "/" are routed to that page. I then created an additional "pages/{id}.js". Now requests to "/" are being routed to "pages/{id}.js" even though pages/index.js is a more specific match.

Also, requests for "/foo/bar" are also getting routed to "pages/{id}.js" - This is not how other frameworks handle this type of routing. I would expect "/foo" to get routed to "pages/{id}.js", but "/foo/bar" to 404.

mikecardwell commented Nov 5, 2016

Using this branch, I created a "pages/index.js". Requests to "/" are routed to that page. I then created an additional "pages/{id}.js". Now requests to "/" are being routed to "pages/{id}.js" even though pages/index.js is a more specific match.

Also, requests for "/foo/bar" are also getting routed to "pages/{id}.js" - This is not how other frameworks handle this type of routing. I would expect "/foo" to get routed to "pages/{id}.js", but "/foo/bar" to 404.

@mikecardwell

This comment has been minimized.

Show comment
Hide comment
@mikecardwell

mikecardwell Nov 5, 2016

Also. I created "pages/foo/index.js", and now requests for "/foo/" get routed there, but "/foo" get routed to "pages/{id}.js".

mikecardwell commented Nov 5, 2016

Also. I created "pages/foo/index.js", and now requests for "/foo/" get routed there, but "/foo" get routed to "pages/{id}.js".

@kawaz

This comment has been minimized.

Show comment
Hide comment
@kawaz

kawaz Nov 9, 2016

I think that it is not good to use parameter names for file names and paths. Because it causes you to do ls to find _xxx.js. For example, in a directory containing 100,000 files, performance will be problematic.

I think that it is better to statically determine the name of the file to search from the request URL. I have an idea.

  |--users
  |    |--_params.js ... `export default { path: "{id}", name: "id" }` 
  |    |--{id}.js ... this file name determ statically by definition of _params.js, don't need ls.
  |    |--{id}
  |        |--foo.js

Developers can freely determine parameter names and path names.

  |--users
  |    |--_params.js ... `export default { path: "_id", name: "userId" }` 
  |    |--_id.js ... url is /users/:userId
  |    |--_id
  |        |--foo.js ... url is /users/:userId/foo

And catch all parameter.

  |--users
  |    |--_params.js ... `export default [ { path: "_id", name: "userId", pattern: /^[0-9]+$/ }, { path: "_actions", name: "action" } ]` 
  |    |--me.js ... url is /users/me
  |    |--_actions.js ... url is /users/:action
  |    |--_id.js ... url is /users/:userId
  |    |--_id
  |        |--foo.js ... url is /users/:userId/foo
  |-- files/_params.js ... `export default { path: "download", name:"path", pattern: /.+/ }`
  |-- files/download.js ... url is /files/:path ( path matches greedy )
  • /users/me can find statically.
  • /users/123 can not find.
    • /users/_params.js is found, and match first pattern.
    • path is /users/_id, and /users/_id.js is found.
  • /users/happy can not find statically.
    • /users/_params.js is found. and match second pattern.
    • path is /users/_actions, and /users/actions.js is found ( The default of pattern is /[^\/]+/ )
  • /users/123/foo can not find.
    • /users/123/ can not find.
    • /users/_params.js is found. and match first pattern.
    • path is /users/_id, and /users/_id/foo.js is found.
  • /files/foo/bar/baz.jpg can not find.
    • /files/foo/bar/ can not find.
    • /files/foo/ can not find.
    • /files/ can find.
    • /files/_params.js is found, and matched.
    • path is /files/download, /files/download.js is found. params.path is foo/bar/baz.jpg.

kawaz commented Nov 9, 2016

I think that it is not good to use parameter names for file names and paths. Because it causes you to do ls to find _xxx.js. For example, in a directory containing 100,000 files, performance will be problematic.

I think that it is better to statically determine the name of the file to search from the request URL. I have an idea.

  |--users
  |    |--_params.js ... `export default { path: "{id}", name: "id" }` 
  |    |--{id}.js ... this file name determ statically by definition of _params.js, don't need ls.
  |    |--{id}
  |        |--foo.js

Developers can freely determine parameter names and path names.

  |--users
  |    |--_params.js ... `export default { path: "_id", name: "userId" }` 
  |    |--_id.js ... url is /users/:userId
  |    |--_id
  |        |--foo.js ... url is /users/:userId/foo

And catch all parameter.

  |--users
  |    |--_params.js ... `export default [ { path: "_id", name: "userId", pattern: /^[0-9]+$/ }, { path: "_actions", name: "action" } ]` 
  |    |--me.js ... url is /users/me
  |    |--_actions.js ... url is /users/:action
  |    |--_id.js ... url is /users/:userId
  |    |--_id
  |        |--foo.js ... url is /users/:userId/foo
  |-- files/_params.js ... `export default { path: "download", name:"path", pattern: /.+/ }`
  |-- files/download.js ... url is /files/:path ( path matches greedy )
  • /users/me can find statically.
  • /users/123 can not find.
    • /users/_params.js is found, and match first pattern.
    • path is /users/_id, and /users/_id.js is found.
  • /users/happy can not find statically.
    • /users/_params.js is found. and match second pattern.
    • path is /users/_actions, and /users/actions.js is found ( The default of pattern is /[^\/]+/ )
  • /users/123/foo can not find.
    • /users/123/ can not find.
    • /users/_params.js is found. and match first pattern.
    • path is /users/_id, and /users/_id/foo.js is found.
  • /files/foo/bar/baz.jpg can not find.
    • /files/foo/bar/ can not find.
    • /files/foo/ can not find.
    • /files/ can find.
    • /files/_params.js is found, and matched.
    • path is /files/download, /files/download.js is found. params.path is foo/bar/baz.jpg.
@kawaz

This comment has been minimized.

Show comment
Hide comment
@kawaz

kawaz Nov 9, 2016

My performance problem (many files in directory) is only in development. (with hot loading)
But if next build can make static routing rule, the problem can resolve?

kawaz commented Nov 9, 2016

My performance problem (many files in directory) is only in development. (with hot loading)
But if next build can make static routing rule, the problem can resolve?

@timoxley

This comment has been minimized.

Show comment
Hide comment
@timoxley

timoxley Nov 15, 2016

Contributor

@nodegin bump, looks like there's some conflicts to fix

Contributor

timoxley commented Nov 15, 2016

@nodegin bump, looks like there's some conflicts to fix

@nodegin

This comment has been minimized.

Show comment
Hide comment
@nodegin

nodegin Nov 15, 2016

Contributor

@timoxley I believe this PR is out of date and Next.js probably would go for another better solution such as a routes.js. However if there's anyone wanna help to fix these conflicts are welcome to reach me by leaving the username for being collaborator in my forked repo.

Contributor

nodegin commented Nov 15, 2016

@timoxley I believe this PR is out of date and Next.js probably would go for another better solution such as a routes.js. However if there's anyone wanna help to fix these conflicts are welcome to reach me by leaving the username for being collaborator in my forked repo.

@tlivings

This comment has been minimized.

Show comment
Hide comment
@tlivings

tlivings Nov 16, 2016

@nodegin can you provide details on the HMR bug?

tlivings commented Nov 16, 2016

@nodegin can you provide details on the HMR bug?

@mikecardwell

This comment has been minimized.

Show comment
Hide comment
@mikecardwell

mikecardwell Nov 16, 2016

There are also the bugs that I mentioned. I don't think this is just a case of fix the conflicts and then release, IMHO the numerous bugs need fixing first.

mikecardwell commented Nov 16, 2016

There are also the bugs that I mentioned. I don't think this is just a case of fix the conflicts and then release, IMHO the numerous bugs need fixing first.

@nodegin nodegin closed this Nov 16, 2016

@kachkaev

This comment has been minimized.

Show comment
Hide comment
@kachkaev

kachkaev Nov 25, 2016

Contributor

The conversation was moved to #291 (if I'm not wrong - just a newbie here)

Contributor

kachkaev commented Nov 25, 2016

The conversation was moved to #291 (if I'm not wrong - just a newbie here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment