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

Adding a JSHint task, and associated live reloading tasks. #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bigcat
Copy link

@bigcat bigcat commented Jan 22, 2014

Added a JSHint task to grunt, and updated watch tasks to cause JSHint to
be re-run every time a js file is saved in the directory. And, added
that "open" flag to the server command, so the site auto-loads in your
default web browser.

Finally, this updates the the default version of dojo to the latest
patch release (1.9.2).

Added a JSHint task to grunt, and updated watch tasks to cause JSHint to
be re-run every time a js file is saved in the directory.  And, added
that "open" flag to the server command, so the site auto-loads in your
default web browser.

Finally, this updates the the default version of dojo to the latest
patch release (1.9.2).
@bigcat
Copy link
Author

bigcat commented Jan 29, 2014

Any chance you could take a look at this - I'm in the middle of doing dojo training for a team moving to it, and they would love to use this w/o tapping my branch.

@bryanforbes
Copy link
Owner

Sorry for the delay. I have a few questions/comments:

  • I'm unsure of the usefulness of livereload for applications. I'd rather reload my application manually when it's needed than have a task do it for me when I might, but more than likely might not, need it.
  • Is there a reason that jshint needs to be run when files change? If a file changes, that means it has been saved and is most likely open in an editor. I find jshint integration in my editor more useful than a task running in the background that I have to tab over to see the results of. I'm also unsure why jshint needs to run for the build task when, by the time the build runs, the user should have had all of their files checked anyway.
  • This seems to have added quite a few new dependencies to _package.json in the app template. Are all of them needed?
  • The indentation for the changes you've made in Gruntfile.js does not conform to the settings outlined in .editorconfig at the root of the project.

@bigcat
Copy link
Author

bigcat commented Jan 29, 2014

Awesome - thanks a ton for getting back to me - sorry if I came off as impatient.

  • So I really like the live-reload personally, but I can definitely see how it can be annoying at times, or not useful for everyone. It is a default in a few of the other yeoman generators, like the webapp one, which is where I got the idea of putting it in, but a few guys on our team have been playing w/ the version of livereloading built into brackets instead. Maybe I could put a prompt there that asks if you want it during the generator phase and make it conditional?
  • So our team has historically done our linting prior to testing in debug or release - and I was trying to mirror that. The idea for us is that we aren't dependent on what editor/IDE you are using or what plugins you use (and the eclipse one is still unofficial and not always supported, which is a popular IDE here to use on our JS team), and forces a common build/debug procedure so we know they have passed through linting before any commits happen. Again, we could make this optional, but is more linting a bad thing?
  • the grunt-contrib-jshint one at first is hopefully obvious - the second jshint one (jshint-stylish) is for making the output look pretty in the console when it lints. I find the output stands out a lot better in the console w/ this as opposed to without. The grunt-newer dependency is so we can only lint the files that changed while debugging. And the time-grunt one is so we can time the build, or individual phases of the build. So the jshint-stylish, and time-grunt would be optional, but otherwise they are necessary to add the linting tasks. I could make this optional as well, but I think it isn't a huge deal to have it there either.
  • My Bad - Our internal coding standards are 2-spaces per tab, so sorry I missed that. If we can decide on a path forward w/ the other questions above, then I can submit another commit that fixes this.

Let me know what you think - thanks again for looking into this.

@neonstalwart
Copy link

So our team has historically done our linting prior to testing in debug or release - and I was trying to mirror that.

most source control management tools give you hooks to trigger processes when code is being integrated into your central repository. a lot of people use these hooks to enforce linting and reject commits that don't pass. this way, the code in the central repository always conforms to your standards. whatever a developer uses to help them comply before they integrate their code with the central repo is up to them (IDE, cli, etc) but the hook becomes a gatekeeper that doesn't let code into your central repo without it conforming to your standards. this is usually a more desirable approach than running linting as part of testing or building since it catches it sooner and doesn't require you to do anything more than just use version control - i.e. you don't have to also run tests or builds to determine if code passes linting.

@bigcat
Copy link
Author

bigcat commented Jan 29, 2014

Thanks for that suggestion, I was aware using hooks for various things but had never heard of linking to the lint/commit rejection process. And now I would like to someday work that into our process. But that isn't what we are currently using internally, and I don't have control over that at the moment to change.

But in my opinion the workflow that lints prior to debug or release build is no less valid or more complicated anyway. Anecdotally my limited experience so far with yeoman suggests that I'm not the only one who uses this workflow (this is the first generator I've used that doesn't lint by default).

So I'm more than open to making it an option with a prompt, so ppl can have a simpler setup if they want.

Prompts during install/yeoman running the generator, asking if you want
live reloading and JSHint to run regularly.
@bigcat
Copy link
Author

bigcat commented Feb 11, 2014

OK, so I made the JSHint and Live Reloading tasks optional, and created prompts to ask if you want them or not.

Let me know if I missed anything or if there is an issue with this?

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

Successfully merging this pull request may close these issues.

3 participants