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

Very simple express-module subgenerator #131

Closed

Conversation

senpairamen
Copy link

This is my very first yeoman generator and my very first contribution to an open source GitHub project. Feel free to reject this Pull Request if it doesn't comply to your standards, this was more as a test for me to get to know yeoman and GitHub forking. This pull request is merely to avoid possibly usable work to go wasted.

By using command yo meanjs:express-module a module is created using the vertical folder structure.
I used the meanjs article module as an example for the templates.

Feedback would be highly appreciated! Thanks

@@ -33,7 +33,8 @@
"chalk": "~1.1.1",
"underscore.string": "~3.2.2",
"yeoman-generator": "~0.20.3",
"yo": "~1.4.8"
"yo": "~1.4.8",
"mkdirp": "~0.5.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why to we need mkdirp and not just use the built in file system functions?

http://yeoman.io/authoring/file-system.html

Also, if we do need mkdirp, it needs to be in alphabetical order. Easiest way is to use npm install mkdirp --save.

@codydaig
Copy link
Member

codydaig commented Oct 1, 2015

@senpairamen Thanks a bunch for this! I left a handful of line comments for you to address before it can get merged in. Overall, great job!

@senpairamen
Copy link
Author

Thanks a lot for the feedback!

},

createFolders: function() {
mkdirp("modules/" + this.name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the slugified version of the module name.

@codydaig
Copy link
Member

codydaig commented Oct 4, 2015

@senpairamen What's the reasoning for keeping mkdirp instead of just using the build in file system?

@ilanbiala ilanbiala self-assigned this Oct 4, 2015
@senpairamen
Copy link
Author

@codydaig I used mkdirp based on this answer: http://stackoverflow.com/questions/13696148/node-js-create-folder-or-use-existing

Tomorrow I will look further into the latest comments and maybe try to write the individual (model, controller, routes, policies) express sub-generators if u want me to.

@codydaig
Copy link
Member

codydaig commented Oct 5, 2015

@ilanbiala do you have a preference for the built in file system or mkdirp? Personally I'd prefer the built in but I'll leave that up to you.

@ilanbiala
Copy link
Member

@codydaig yeoman's this.fs doesn't expose a directory creation method, and the creator of the package even suggested using mkdirp, so I guess it's fine. What do you mean by built-in? Are you referring to Node's fs?

@codydaig
Copy link
Member

codydaig commented Oct 5, 2015

@ilanbiala @senpairamen My apologies. I just skimmed and misread the yeoman file system docs, thought it had an option to make a directory.

@MichaelGatesDev
Copy link

Does this not generate the 'client' folder as well?

@senpairamen
Copy link
Author

@MichaelGatesDev no, this is the express subgenerator so I believe it should only create the files and folders for the server end. Correct me if i'm wrong.

@senpairamen
Copy link
Author

Once this one gets merged in, I will start working on the model, server, controller, policies individual express subgenerators.

@ilanbiala
Copy link
Member

@senpairamen I'd rather not have too many subgenerators, and I think deleting the client files instead of running and maintaining a separate generator might be better.

@codydaig @senpairamen @MichaelGatesDev thoughts?

@senpairamen
Copy link
Author

@ilanbiala @codydaig I'm with you on this one.

Why not create the whole CRUD module and then ask the user which parts are needed, both Angular and Express? That way, generating individual parts are still available.

@MichaelGatesDev
Copy link

I think that sounds good @ilanbiala @senpairamen . I'm fairly new to Node & Express & the mean stack, so I don't really know if there's something else to suggest. So I'll just say yeah haha. Thanks.

@ilanbiala
Copy link
Member

Ah okay.

@mleanos
Copy link
Member

mleanos commented Oct 11, 2015

@senpairamen What's the current state of this PR? I see the comment about you losing code.

Could you provide some brief points that might be helpful, if someone were to come along and continue where you left off? Also, a brief explanation of the code that was lost?

@senpairamen
Copy link
Author

@mleanos I hope I can redo this this weekend. I'll keep you posted.

@ilanbiala ilanbiala mentioned this pull request Oct 26, 2015
@senpairamen
Copy link
Author

I cant get the file deletion to work, either async or not they are not getting removed. I have a feeling the script is trying to remove the files before they are done being copied.

Furthermore, I dont think creating prompts for every angular/express part is very efficient. To support every subgenerator we'd have to create like 10+ prompts (personally, I find that a bit overkill).

mkdirp = require('mkdirp'),
log = require('../app/log');

var exec = function (cmd) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just extract this into a helper file and also refactor it out of the index.js

@ilanbiala
Copy link
Member

Good progress, left some comments. Ideally we are generating a full module, so let's try not to leave stuff empty. Also, let's add some tests to this to make sure it works as expected.

@senpairamen
Copy link
Author

The functionality has been copied from the articles example but emptied out the functions as that could create non-working code?

Regarding the tests, I have never written any test in JavaScript thus far. I'd rather have someone else write those

Edit: i'll check on the tests later to see how difficult they are to write, if it isn't very time consuming i'll write them myself

@ilanbiala @codydaig @MichaelGatesDev Personally, I think we should create 3 subfolders (crud-module, angular-module, express-module). The crud-module generator just generates both the express-module and angular-module. Inside the angular-module and express-module subgenerators we prompt the user for the individual parts. That way we have all the generators available but we don't have to prompt the user 10+ times before something gets generated because that is what happens if we leave the structure like this (1 subfolder called crud-module).

@ilanbiala
Copy link
Member

@senpairamen the separate module subgenerators sounds good to me.

@soupman99 soupman99 mentioned this pull request Nov 21, 2015
@ilanbiala
Copy link
Member

@senpairamen any progress on this? We'd definitely love to add it in.

@satyarapelly
Copy link

Do we have Sub-module generator available with latest code check-in?
which version of generator-meanjs to be installed?

@UndeadBaneGitHub
Copy link
Contributor

Excuse me, but this "create full crud module and then delete the unchecked files" approach does really sound sad from user's perspective - although I do understand, it does sound attractive from developer's.

The reasons:

  1. Many, MANY checkboxes to check to generate ONLY, say, angular controller
  2. Potential risk of overwriting user's files - so a lot of checks must be written
  3. Overally slower generation - more FS operations.

I mean, having a crud generator is nice, but with other generators, not instead of it.

Also, I've PRed all the submodule generators for 4.2.0 except for

  • that very CRUD
  • ACL generator

PR is here: #157

@ilanbiala
Copy link
Member

@UndeadBaneGitHub this PR can be closed since your PRs were merged in, right?

@UndeadBaneGitHub
Copy link
Contributor

Yep!

On 23 янв. 2016 г., at 5:06, Ilan Biala notifications@github.com wrote:

@UndeadBaneGitHub this PR can be closed since your PRs were merged in, right?


Reply to this email directly or view it on GitHub.

@ilanbiala ilanbiala closed this Jan 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants