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

Nested routing support #20

Closed
rjharmon opened this issue Feb 2, 2017 · 20 comments
Closed

Nested routing support #20

rjharmon opened this issue Feb 2, 2017 · 20 comments

Comments

@rjharmon
Copy link

rjharmon commented Feb 2, 2017

In the current rekit architecture, a feature provides only a single layer of routing; common/routeConfig.js combines a flat set of feature routes. Naturally, pages do not have nested routes.

For a complex application, Rekit's modularized approach on managing components and containers is really helpful. If features could be more naturally nested, it would really help managing bigger applications. I imagine features/home/all-about-us/ having a variety of pages, with the routes being nested under the home router. Ideally, those nested routes are lazy-loaded with react-router's getComponent() and webpack's code splitting

@supnate have you done any design thinking along that line?

@supnate
Copy link
Owner

supnate commented Feb 2, 2017

Hi @rjharmon , thank you very much for the feedback!

Rekit uses react-router's JavaScript API to define routing rules, so it has supported nested routing by default: https://github.com/ReactTraining/react-router/blob/master/docs/guides/RouteConfiguration.md . It should also with lazy load by Webpack's code splitting.

For example, in home/route.js you could have below definition to support nested routes:

export default {
  path: '',
  name: 'Home',
  childRoutes: [
    { path: 'home', name: 'Home page', component: HomePage, isIndex: true },
    { path: 'about', childRoutes: [
        { path: 'us', component: AboutUs },
        { path: 'help', component: HelpPage },
    ] },
  ],
};

For nested features it looks good to me too but I'm afraid it will add too much complexity to either Rekit or the created application. Instead I'm thinking about to support grouping components/actions into different folders. Currently it only supports flat files by Rekit tools.

And since you are trying Rekit (I'm very glad to know that) I want to share my current work on Rekit 2.0: It will allow to rename Rekit elements such as components, actions, features etc. It will provide Web UI named rekit-portal to manage a Rekit project. And there will be no root containers/components folders but will add a default feature names common where you put shared components or containers besides home. And there will be no page concept but you can add a component optionally with connect to redux or defined in router. This is not breaking changes compared to Rekit 1.0 so projects created by v1.0 should be very easy to migrate to Rekit 2.0.

I think with these changes there will be less concepts and more consistence.

If you want to have an early look at Rekit 2.0, you could visit: https://github.com/supnate/rekit-portal . It is a web application created by Rekit itself to mange a Rekit project. By looking at rekit-portal you could have a overview of how a Rekit 2.0 application works. You can check out the rekit-portal repo and run npm install, npm start then access http://localhost:6078.

Hope you will enjoy Rekit 2.0:-) And thanks again for sharing your thoughts with me.

@rjharmon
Copy link
Author

rjharmon commented Feb 2, 2017

hey, that's pretty cool-looking. Looks like the dependency on refactor may need freshening in rekit-core maybe? getFeatureStructure doesn't seem to be available for api/fetchProjectData.js.

Also:

$ npm start

> rekit-portal2@0.0.1 start C:\Users\rjhar\dev\rekit-portal
> node ./tools/server.js

C:\Users\rjhar\dev\rekit-portal\src\middleware\api\fetchProjectData.js:7
const { refactor, utils } = rekitCore;
      ^

SyntaxError: Unexpected token {
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:373:25)
    at Object.Module._extensions..js (module.js:416:10)
    [trimmed]

@supnate
Copy link
Owner

supnate commented Feb 2, 2017

It seems rekit-core@2.0-alpha.5 was not published successfully.
To get a quick fix, would you please try a local rekit-core? Follow below steps:

  1. Clone https://github.com/supnate/rekit-core to a local folder.
  2. cd rekit-core folder run: npm install and then run npm link
  3. cd rekit-portal folder run: npm link rekit-core.

You can get to know npm link at https://docs.npmjs.com/cli/link if you are not familiar with it.

Then you could use the local rekit-core project, it should then work. Good luck!

@rjharmon
Copy link
Author

rjharmon commented Feb 2, 2017

Thanks, will check it out.

Here is a little more on my line of implementation thinking for nested features:

  1. Adjust features/foo/route.js (its template) to look just like common/routeConfig.js:
var childRoutes = [
  { path: 'detailed', name: 'Detailed diagram', component: DetailedDiagram },
]

export default {
  path: 'diagram',
  name: 'Diagram',
  childRoutes: childRoutes
};
  1. When creating a nested feature like features/home/about/, instead of inserting directly to features/common/routeConfig.js, just find its parent router in home/about/../route.js (if not found, fall back to ../../route.js or ultimately the root routeConfig.js the way you do now). Whichever router is the closest parent would be updated using the same logic you already have.

  2. Consider moving features/common/routeConfig.js to features/route.js, This would help the "root feature" (== the app) be shaped the same (for router-defs) like any other feature, and remove the special case from step 2.

  3. Consider renaming features/ to app/ (leave a symlink for compatibility)

Probably there are other issues besides the route-creation sequence that are not as easy to adjust - input is welcome.

@supnate
Copy link
Owner

supnate commented Feb 2, 2017

Thanks for you proposes. Look good to me, especially NO 4, renaming features to app makes much sense. However feature is a heavy artifacts in Rekit, supporting nested features will have much influence to the existing architecture. I will seriously consider the value and complexity brought by it.

BTW, I just realized the it's a syntax error for you to run rekit portal. You are using node v4.x which doesn't support const { a, b } = c;? Simply change it to const refactor = rekitCore.refactor; const utils = rekitCore.utils. Rekit portal was intended to support node v4.x so I will also fix it. Of course you can also try to upgrade node to v6.x or using nvm to switch versions.

@rjharmon
Copy link
Author

rjharmon commented Feb 2, 2017

Yeah, it was a node version issue, since fixed. Thanks for the tech support. :/

Hah, I hesitated to suggest renaming features/ to app/, even with a compatibility symlink.

I'd be happy to contribute to a possible change. I'd like to get a sense of what other (non-routing) concerns are likely to be troublesome.

@supnate
Copy link
Owner

supnate commented Feb 2, 2017

haha, you are maybe the first 'lucky' guy to see a running rekit-portal:-P

Currently I would like to put releasing Rekit 2.0 at the first priority. So that we could hear more feedbacks about whether nested features are also needed by most people. My concern is still it will add complexities to both Rekit and applications sides. Forcing a flat structure makes project easy to understand in my opinion. If it's heavily required, I can add it in the next major release. Actually the requirement from you is more like a nested routing support.

And thanks for your willing to contribute, you may check out rekit-core to understand the system, and understand why nested features are complicated (for now I feel the key reason is just it adds more patterns for auto detect project folder structure), it provides core APIs for managing a Rekit project.

@supnate
Copy link
Owner

supnate commented Feb 2, 2017

BTW...

I guess you can't expand project explorer tree...

It's caused by antd lib which doesn't work well with react hot loader. Need some trick:

  1. Delete .tmp folder
  2. Navigate to rekit-portal/node_modules/rc-tree/lib/TreeNode.js.
  3. Go to line 238 around, change the code to below:
    if (children && (children.type && children.type.isTreeNode || Array.isArray(children) && children.every(function (item) {
      return item.type && item.type.isTreeNode;
    }))) {

Then re-run rekit-portal.

@rjharmon
Copy link
Author

rjharmon commented Feb 6, 2017

Is it normal for fetchProjectData to have item.file containing a full path (from node's perspective) of a component's (etc) source file? I'd figure it would be specified as URL-relative, but then I was fiddling with rekit-core. Being that I'm running Windows, that might play into it too.. Will do more troubleshooting, but thought I'd check with you

@rjharmon
Copy link
Author

rjharmon commented Feb 6, 2017

And yes, I did get the browser app working, with the file list and graphic. That's pretty sweet, though I admit it's a bit confusing sharing the same application space with the application you're using to visualize and control the application space. Inception, like.

I dig how you're structurally manipulating the source code in response to application metadata changes.

@supnate
Copy link
Owner

supnate commented Feb 6, 2017

Yeah, since Rekit portal is also a Rekit app, it could be managed by itself. It auto detects the project where it's running so it will mange a normal Rekit app which will depends on Rekit portal when 2.0 is final released. Currently you could change rekit-portal/src/middleware/index.jsto manually set the project it manages.

Such as:

rekitCore.utils.setProjectRoot('/Users/nate/workspace2/rekit-app-1');

@supnate
Copy link
Owner

supnate commented Feb 6, 2017

And I've not run Rekit on windows for long, so there may be some issues.

@rjharmon
Copy link
Author

rjharmon commented Feb 6, 2017

src\features\rekit-tools\BuildPage.js seems like the format that the feature's 'file' wants to be in, but haven't traced down the code that is supposed be responsible for generating it that way, or at least not yet.

@supnate
Copy link
Owner

supnate commented Feb 6, 2017

Sorry that I don't understand the question... However I just deployed a demo on Heroku: https://rekit-portal.herokuapp.com/ You may want to check it. The demo app has not intended to be public for now, but you can just try it. It will be in readonly mode later so don't delete or rename elements so that I can show it to other early adopters.

@rjharmon
Copy link
Author

rjharmon commented Feb 6, 2017

OK, from your app, the project-info API call is returning an entry 'misc', along with other entries, all of whose 'file' entries include /app/src/ ...

misc: [{feature: "home", name: "images", type: "misc", file: "/app/src/features/home/images",…},…]

I'd hope for those paths to be relative to the app dir (such as "src/features/home/images"), so that the client-side code doesn't have to munge the path to get rid of the app-root (/app/, here; "c:\Users\rjharmon\dev\react-portal", in my case). I think the URL encoding of : or \ is preventing that correct removal for me. It's quite tangential to this issue #20, though. I'll make a separate ticket if needed.

@rjharmon rjharmon closed this as completed Feb 6, 2017
@rjharmon rjharmon reopened this Feb 6, 2017
@supnate
Copy link
Owner

supnate commented Feb 7, 2017

Thanks for the investigation. Using full path is convenient in many places for rekit-core which frequently interacts with disk files. I should have reminded you that black slash is a common issue for compatibility between windows and mac... To fix that, need to deep dive into rekit-core/core/vio.js where all disk operation happens(read and write). However I will fix windows issues after all main features done.

@supnate supnate closed this as completed Feb 7, 2017
@rjharmon
Copy link
Author

rjharmon commented Feb 7, 2017

I'd suggest using https://github.com/kamicane/pathogen or another utility to more easily work with paths. I understand dealing consistently with pathnames is a convenience, but using full host path names is also a security risk. For example of how this can be used to an attacker's advantage, try this from your browser console from your demo site:

function reqListener () {
  console.log(this.responseText);
}

var oReq = new XMLHttpRequest();
oReq.addEventListener("load", reqListener);
oReq.open("GET", '/rekit/api/file-content?file=%2fetc%2fpasswd')

Recommended: normalize all paths so that they're project-root relative, deal consistently with them in that form; reform full system paths with the project-root only when it's required.

@supnate
Copy link
Owner

supnate commented Feb 7, 2017

Thanks for the reminding about security risk! That really makes sense. I thought rekit-portal always runs on local machine but if others know the ip it's really put user under risk. I will review the middleware API to prevent reading arbitrary files.

Using relative paths doesn't help because rekit-portal needs project root path to display to the user.

I will also look at normalizing paths later. Thanks for the recommendation.

@supnate
Copy link
Owner

supnate commented Feb 7, 2017

Reopen for tracking fullpath issue.

@supnate supnate reopened this Feb 7, 2017
@supnate
Copy link
Owner

supnate commented Feb 12, 2017

Fixed the security issue of fetching file content api by restrict the full path to start with the project root path.

@supnate supnate closed this as completed Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants