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

Provide all URL parameters to each route #57

Merged
merged 1 commit into from Oct 20, 2016

Conversation

@langpavel
Copy link
Collaborator

langpavel commented Oct 10, 2016

Continued effort on #50

Goal: Allow passing parameters from parent route to child routes

Example:

const routes = {
  path: '/:articleId',
  children: [
    {
      path: '/',
      async action({ params }) {
        // can use params.articleId too!
      },
    },
    {
      path: '/edit',
      async action({ params }) {
        // can use params.articleId too!
      },
    },
  ],
  async action({ params }) {
    // can use params.articleId
  }
};

TODO

  • Decide how to enable this behavior in router
  • Think more deeply if not inheriting params from parent routes should be default behavior
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 10, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling eeb5c3e on langpavel:pass-params-explicitly into 7a1c314 on kriasoft:master.

@langpavel

This comment has been minimized.

Copy link
Collaborator Author

langpavel commented Oct 12, 2016

@koistya @frenzzy Any ideas on this?

@frenzzy

This comment has been minimized.

Copy link
Member

frenzzy commented Oct 14, 2016

I agree with @koistya this feature is not necessary for many simple use cases.

But I think it would be nice to have ability to enable it if necessary. I see only two ways were we can add mergeParams option:

  1. To each route, but in this case list of routes may be too verbose:

    const routes = [
      { path: '/:username', action: () => 'Profile',
        mergeParams: true, // <= 1
        children: [
          { path: '/edit', action: () => 'Edit Profile' },
          { path: '/posts', action: () => 'List Posts' },
          { path: '/posts/:uri',
            mergeParams: true, // <= 2
            children: [
              { path: '/', action: () => 'Post' },
              { path: '/reviews', action: () => 'Reviews' },
              { path: '/comments', action: () => 'Comments' },
              { path: '/comments/:id',
                action(context, params) {
                  // using params from top level route here
                  if (context.username === params.username) {
                    return 'Comment (highlighted)'
                  }
                  return 'Comment'
                }
              }
            ]
          }
        ]
      }
    ]

    I mean to pass the param from top-level route to the deepest child, need to add option mergeParams for each route in the middle.

  2. To resolve call, I think this is the preferred way.

    const options = { mergeParams: true }
    
    resolve(routes, '/path', options)
@langpavel

This comment has been minimized.

Copy link
Collaborator Author

langpavel commented Oct 16, 2016

I think the second is better..

@frenzzy

This comment has been minimized.

Copy link
Member

frenzzy commented Oct 20, 2016

I discussed this PR with @koistya and decided to merge params always without an option.

@frenzzy frenzzy merged commit c08ae00 into kriasoft:master Oct 20, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@langpavel

This comment has been minimized.

Copy link
Collaborator Author

langpavel commented Oct 21, 2016

Yippee! 🎉 Thank you a lot @frenzzy and @koistya

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.