Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

stylelint middleware #615

Merged
merged 15 commits into from
Jan 29, 2018
Merged

stylelint middleware #615

merged 15 commits into from
Jan 29, 2018

Conversation

timkelty
Copy link
Contributor

Most of what's here is adapted from https://github.com/barraponto/neutrino-middleware-stylelint 👏

Curious if @mozilla-neutrino/core-contributors think this should be in core, or just maintained by community. If the consensus is the latter, I'm happy to PR @barraponto's package.

stylelint felt like a commonly used enough tool that it could be a good case for a core middleware.

Related: #612

@timkelty timkelty changed the title Stylelint middleware stylelint middleware Dec 13, 2017

test('exposes stylelintrc config', t => {
t.is(typeof Neutrino().use(mw()).call('stylelintrc'), 'object');
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm - This last test causes tests to fail: https://travis-ci.org/mozilla-neutrino/neutrino-dev/jobs/316091388#L920-L932 not sure why…seems identical to the eslintrc test…

Copy link
Member

Choose a reason for hiding this comment

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

Run the tests locally and see the output of Neutrino().use(mw()).call('stylelintrc').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh derp - it's b/c stylelint throws err if you don't pass it anything.
Fixed.

@barraponto
Copy link
Contributor

YAY let's move this into core!

@timkelty
Copy link
Contributor Author

timkelty commented Dec 18, 2017

Is anyone else using Atom/AtomLinter with linter-stylelint?

If I try to use neutrinorc.js for my stylint config, I get this error: atom/atom#15093 (comment)

.stylelintrc.js

const {Neutrino} = require('neutrino');
const {resolve} = require('path');

module.exports = Neutrino()
  .use(resolve(__dirname, '.neutrinorc.js'))
  .call('stylelintrc');
  • neutrino stylelintrc outputs the correct thing from the cli.
  • Sublime Text 3/SublimeLinter works fine.

files: '**/*.+(css|scss|sass|less)',
context: neutrino.options.source,
failOnError: neutrino.options.command !== 'start',
quiet: neutrino.options.command === 'start'
Copy link
Member

Choose a reason for hiding this comment

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

We also have neutrino.options.quiet, would that be useful here?

@eliperelman
Copy link
Member

So far I think this is pretty cool! Could you post some screenshots showing this in action?

@timkelty
Copy link
Contributor Author

Would also like to tie this into the lint command. Haven't had success so far: #612

@@ -0,0 +1,65 @@
# Neutrino stylelint Middleware
Copy link
Member

Choose a reason for hiding this comment

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

Need a docs/ copy of this too.. :-)

Copy link
Member

Choose a reason for hiding this comment

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

Also a reference from docs/SUMMARY.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup - I'll wait till things gel a bit, then add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

@timkelty
Copy link
Contributor Author

Here's some screenshots:
neutrino stylelint command:
screen shot 2017-12-19 at 8 49 11 pm

neutrino start:
screen shot 2017-12-19 at 9 20 57 pm


## Requirements

- Node.js v6.10+
Copy link
Member

Choose a reason for hiding this comment

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

This section needs to be updated according to the other docs as well. I wish we had templated docs.

files: '**/*.+(css|scss|sass|less)',
context: neutrino.options.source,
failOnError: neutrino.options.command !== 'start',
quiet: neutrino.options.command === 'start',
Copy link
Member

Choose a reason for hiding this comment

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

When the command isn't start, what does quiet do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures that the actual error is output, when failOnError is enabled.

E.g. with settings the way they currently are, you will get:

⠼ Building project
src/css/index.scss
 8:1  ✖  Unexpected empty line before at-rule   at-rule-empty-line-before


✖ Building project failed
Error: Failed because of a stylelint error.

    at linterSuccess (/Users/timkelty/Code/neutrino-dev/node_modules/stylelint-webpack-plugin/lib/run-compilation.js:34:14)
    at <anonymous>
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

But if we don't set quiet (defaulting to true), you won't get the actual error:

✖ Building project failed
Error: Failed because of a stylelint error.

    at linterSuccess (/Users/timkelty/Code/neutrino-dev/node_modules/stylelint-webpack-plugin/lib/run-compilation.js:34:14)
    at <anonymous>
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

The other option is to not set either failOnError or quiet, since by default it emits errors to webpack, so the build will fail anyway.

Copy link
Contributor Author

@timkelty timkelty Jan 17, 2018

Choose a reason for hiding this comment

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

The other option is to not set either failOnError or quiet, since by default it emits errors to webpack, so the build will fail anyway.

I opted to go this route. Let me know if you see any issues. I belive I started down the failOnError route because that's what @neutrinojs/eslint does…

const files = join(options.context, options.files);

return lint(merge(options, { fix, files }))
.then(result => Promise[result.errored ? 'reject' : 'resolve'](result.output));
Copy link
Member

Choose a reason for hiding this comment

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

For clarity, and I know it's a little nitpicky, let's move the ternary to the outside:

return lint(merge(options, { fix, files }))
  .then(result => result.errored ?
    Promise.reject(result.output) :
    Promise.resolve(result.output));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nitpicky is good :)

"publishConfig": {
"access": "public"
},
"dependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

@timkelty timkelty changed the title stylelint middleware WIP: stylelint middleware Jan 17, 2018
@timkelty timkelty changed the title WIP: stylelint middleware stylelint middleware Jan 23, 2018
@timkelty
Copy link
Contributor Author

Ok, I think all review notes should be addressed, and I added some documentation for the cli commands.

Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Thank you for your patience on this one!

@eliperelman eliperelman merged commit daafb2d into neutrinojs:master Jan 29, 2018
@timkelty
Copy link
Contributor Author

🍾

@timkelty timkelty deleted the stylelint branch January 29, 2018 17:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

4 participants