flaviait / ng2-jspm-template

A template for a quick development workflow with angular 2 and jspm
MIT License
14 stars 3 forks source link

Lints and watches #25

Closed svi3c closed 8 years ago

svi3c commented 8 years ago

This PR adds linting for styles via stylelint. Also the linter are run on file changes.

Resolves #24 Resolves #23

@DorianGrey

DorianGrey commented 8 years ago

Hm... I see several issues here:

The latter two issues might especially causes problems in larger projects.

svi3c commented 8 years ago

@DorianGrey you are absolutely right! It was just simple to use the CLI. I'll give the linting API a try and use chokidar for change detection and batch the linting for the changed files. Since the linting does not have to check any dependencies, we should be able to lint only the changed files.

svi3c commented 8 years ago

@DorianGrey I just updated the stuff. Please re-check. The linting is very quick now. We keep the errors in memory and update them incrementally. This way we can show all linting errors, even if only one file changed. I think this is better than having only the linting errors for the currently modified file, since there is some output in the dev console which scrolls the older errors out of sight.

DorianGrey commented 8 years ago

I agree - that's one of the problems which regularly occur on most build tools.

But there is on thing that should be added to this PR as well: we've currently restricted the node engine version to >= 4.0.0. Trying to use this version causes a warning:

npm WARN engine stylelint@7.1.0: wanted: {"node":">=4.2.1"} (current: {"node":"4.0.0","npm":"2.14.2"})

The engine restriction should be raised to something like (NPM > 3 should be preferred over 2.x anyway):

"engines": {
  "node": ">=4.5",
  "npm": ">=3.9"
}

Otherwise, things might fail to work in several circumstances - I don't think the styleline-devs set this restriction for no reason. Maybe providing a .nvmrc would be useful as well.

Another warning that (curiously) only occurred when using 4.0.0 was this:

npm WARN package.json Dependency 'http-proxy' exists in both dependencies and devDependencies, using 'http-proxy@^1.14.0' from dependencies

Which holds true. I'm wondering why NPM 3.10 did not complain about this lately, but we should try to avoid this warning anyway.

svi3c commented 8 years ago

Okay, done :)

DorianGrey commented 8 years ago

I've reverted the change to the zone.js dependency and added the commit to this branch. Suppose this change happend by accident (caused by jspm install - happend to me as well...). W.r.t. its issue list, we should not allow a variable range for this dependency for now. E.g. https://github.com/angular/zone.js/issues/407 - I've also seen this while updating dependencies for RC.5 and reverted to 0.6.12.

Everything else looks fine and will be merged once Travis is ready.

€dit: I've update the .travis.yml as well, to match the changed engine restrictions.