cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.07k stars 399 forks source link

[lint-jss] warn about silly mistakes #255

Open jacksonrayhamilton opened 8 years ago

jacksonrayhamilton commented 8 years ago

When I use the API like this:

var jss = require('jss');
var sheet = jss.createStyleSheet({
  '.foo': {
    bar: 0
  }
});
console.log(sheet.toString());

...then my initial expectation was to get:

.foo {
  bar: 0;
}

...but instead, I get:

..foo-1028573841 {
  bar: 0;
}

I now know that I need to pass {named: false} to the createStyleSheet to generate the correct class name.

The same issue occurs when one forgets to use a plugin, e.g. if jss-camel-case is not used, then camel-cased properties will be generated without so much as a warning.

Another case of this occurs with jss-nested; coming from SCSS, I initially expected this:

var jss = require('jss');
var nested = require('jss-nested').default;
jss.use(nested());
var sheet = jss.createStyleSheet({
  'foo': {
    '> *:first-child': {
      'bar-bar': 0
    }
  }
});
console.log(sheet.toString());

...to generate:

.foo-3691883159 > *:first-child {
  bar-bar: 0;
}

...but instead, it generates:

.foo-2511288833 {
  > *:first-child: [object Object];
}

I should have used the syntax & > *:first-child (with the &). The ampersand was implied in SCSS, but omitting it in JSS resulted in strange output.

It seems that non-sensical class name generation can be a "gotcha" for those new to JSS.

I think it'd be helpful if JSS could detect if it's generating malformed CSS. Maybe this functionality could be available as a development-only plugin, so performance wouldn't need to suffer. It could be as simple as detecting the use of basic CSS syntax / weird casing in keys and printing warnings, or as thorough as a "JSS Lint" utility with knowledge of all valid property names. What do you think?

kof commented 8 years ago

Yep, agreed, good idea.

kof commented 8 years ago

I think we can even utilize any existing css linter, additionally to jss specific linting, and lint generated css on the fly. For dev only, of course.

jacksonrayhamilton commented 8 years ago

A CSS linter would be a good start, and probably get us error-checking fairly quickly.

However, if we only used a CSS linter to analyze the output, the user would just be told what went wrong with the CSS, rather than precisely which JSON property caused malformed CSS. So it might be good to do checking not just on the output, but also during CSS generation. (Not sure if that's what you meant by "additionally to jss specific linting.")

kof commented 8 years ago

Yes the problem might be to have a meaningful explanation where exactly what was wrong if we analyze css output, hopefully we can at least track it down to the rule.

kof commented 8 years ago

We are talking about 2 things: one is jss syntax checker for popular errors, second is a css linter.

jacksonrayhamilton commented 8 years ago

Yep, they would be two different things. Both seem relatively easy to implement. You described the CSS linter. The "JSS syntax checker" could potentially be embedded inside JSS and its plugins in the form of if blocks that throw errors, where those blocks might be stripped out in production by a dead code eliminator. (For an example of this, see the React source code: [1], [2])

kof commented 8 years ago

What is the benefit to add it to jss? I want to keep the core small. I would rather see it as a separate package.

jacksonrayhamilton commented 8 years ago

Looks like that checking could work as a package, since the plugin API provides access to rule.style, which we could examine.

A question that lingers is, where do plugin-specific checks go?

So all the checks for all the plugins could potentially be in one "jss-lint" plugin. Which has the potential to be messy, but then again it might be just fine, especially if we limited the linting to the core and the plugins maintained by the jsstyles organization.

kof commented 8 years ago

Yeah, it would make sense to maintain plugin related checks within plugin package. So that once plugin changes, it immediately changes it's linter.

kof commented 8 years ago

How about this:

  1. one linter package
  2. it lints the core
  3. every jss plugin can also implement an optional linter plugin, basically plugins can plug-in to jss as well as into linter. It can check the absence of itself in the setup as well as malformed rules
  4. linter uses all plugins packages as dependencies

Example of such a plugin with interface:


export default function jssExtend() {
  return (rule) => {...}
}

export function jssExtendLinter() {
  return (rule) => {...}
}
jacksonrayhamilton commented 8 years ago

Seems pretty good. Maybe the linting code should be in a separate file, though? (Stored in lint.js, alongside the plugin's index.js.) That way, the linting code wouldn't affect the size of the production bundle.

jacksonrayhamilton commented 8 years ago

As for the CSS linter used on the generated CSS output: Should we pick one, or allow that to be customized, too?

It might be easier to set up just one CSS linter. Although, it might be harder for users to configure it, and also the choice would be opinionated. (I've actually only used SCSS linters, so I'm not in a position to judge.)

If the CSS linter were to be customizable, the user would have maximum freedom in that regard, but we'd have to define a compatibility interface (probably the user specifies a function that takes a CSS string as input and reports errors in a format that jss-lint understands, presumably by calling a CSS linter's programmatic interface and formatting the results; maybe we could support certain linters out-of-the-box, too). Also, some linters like stylelint return a Promise, so the API should probably support that, too.

kof commented 8 years ago

https://github.com/stylelint/stylelint

kof commented 7 years ago
bytasv commented 5 years ago

Hey guys, does linting currently work in any way?

kof commented 5 years ago

Nope, so far nobody had time to work on it.