Angular route subgenerator and test added

[UndeadBaneGitHub]

No description provided.

[codydaig]

New Line

[codydaig]

Left some line comments.
Don't forget to squash commit.
Otherwise, LGTM.

[UndeadBaneGitHub]

@codydaig Regarding new line in the end of angular-route/templates/_.client.route.js - there should be none, as this template is inserted during the generation into an existing file, and newline will break the code.
Also, I found a bug in template, fixing.

[ilanbiala]

newline

[UndeadBaneGitHub]

Again, why newline in a partial, that is used to replace a
$stateProvider. string in original file?
Newline in the end would just ruin the idea.

2015-12-21 1:38 GMT+03:00 Ilan Biala [email protected]:

In angular-route/templates/_.client.route.js
#162 (comment)
:

@@ -0,0 +1,5 @@
+$stateProvider.

• state('<%= slugifiedName %>', {
• url: '/<%= slugifiedRoutePath %>',
• templateUrl: 'modules/<%= slugifiedModuleName %>/client/views/<%= slugifiedViewName %>.client.view.html'
• }).

newline

—
Reply to this email directly or view it on GitHub
https://github.com/meanjs/generator-meanjs/pull/162/files#r48109358.

[ilanbiala]

Where do this get pulled into that makes it a partial? Also, why does it end with a .?

[UndeadBaneGitHub]

@ilanbiala It ends with a . exactly because it's a partial. Look up _.client.routes.js - that's a template for routes file, which should contain many of them. If the file already exists, this partial is taken instead, and inserted into an already existing routes file. See index.js, line 89 and below.

[ilanbiala]

Where is this implemented? How does it work? What is the goal and the end result?

[UndeadBaneGitHub]

Please, see angular-route/index.js, renderRoute function:

  renderRoute: function () {
    var routesFilePath = process.cwd() + '/modules/' + this.slugifiedModuleName + '/client/config/' + this.slugifiedModuleName + '.client.routes.js';

    // If routes file exists we add a new state otherwise we render a new one
    if (fs.existsSync(routesFilePath)) {
      // Read the source routes file content
      var routesFileContent = this.readFileAsString(routesFilePath);

      // Append the new state
      routesFileContent = routesFileContent.replace('$stateProvider.', this.engine(this.read('_.client.route.js'), this));

      // Save route file
      this.writeFileFromString(routesFileContent, routesFilePath);
    } else {
      this.template('_.client.routes.js', 'modules/' + this.slugifiedModuleName + '/client/config/' + this.slugifiedModuleName + '.client.routes.js')
    }
  }

The code is quite well commented, but I will repeat the idea here:

1. For module name foo check, that foo.client.routes.js already exists
   2a. If it does, render the partial _.client.route.js and insert the rendered contents with new route description into an existing foo.client.routes.js
   2b. If it does not exist, render template _.client.routes.js and save rendered result as foo.client.routes.js

[ilanbiala]

@UndeadBaneGitHub @codydaig shouldn't any code be based on our using ui-router and not ng-route? I don't think we are using ng-route at all...

[UndeadBaneGitHub]

@ilanbiala Ehm... But it is based on ui-router, hence $stateProvider. Old ngRoute uses $routeProvider.

[ilanbiala]

Oh sorry, I thought you were actually referring to angular-route code..my mistake. Can you briefly detail all the possible scenarios and what happens? I just want to make sure we didn't miss one and there is nothing better for each scenario.

[codydaig]

@ilanbiala LGTY?

[ilanbiala]

Only thing I'm worried about is some sort of scenario not working? @UndeadBaneGitHub can you maybe add a test for each scenario? For example, for the one where a file already exists, maybe create a file in the test that has xyz code, and then expect abc code after? That will ensure it works properly even in those cases.

[UndeadBaneGitHub]

Fixed! Although, need help with squashing: if I do it, then conflicts emerge, because pull requests have been merged, which is kinda strange.

[ilanbiala]

newline

[ilanbiala]

@UndeadBaneGitHub how are you doing the squash?

[UndeadBaneGitHub]

Normally, like this: git reset --soft HEAD~2 && git commit, but it is unapplicable because of the merge

[ilanbiala]

Oh right, you need to get rid of those couple files. I usually do git rebase -i HEAD~<<number of commits back>> and then change all commits after the top one to fixup and the top one to reword and that usually works for me. However, that won't get rid of the files, it just does the squash for you.

[UndeadBaneGitHub]

@ilanbiala ok, I managed to squash the commits into one now. Waiting for the merge to fix timeout(0) and temp folder!

[ilanbiala]

Why are these files being deleted?

[ilanbiala]

LGTM other than all the deleted files? Those don't look relevant to this PR, are they a result of rebasing/squashing/merging?

[UndeadBaneGitHub]

@ilanbiala actually, let me check about the deleted files.

[UndeadBaneGitHub]

@ilanbiala Ok, I just forgot to merge master into this branch. Now it's fixed and the files are back

[ilanbiala]

@UndeadBaneGitHub The comment is still there and there are a bunch of wholly deleted files, did you forget to push or finish the merge or something?

[UndeadBaneGitHub]

Created a new PR #183, closing this one.