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

Release proposal: standard v8 #564

Closed
16 tasks done
feross opened this issue Jul 12, 2016 · 71 comments · Fixed by #603
Closed
16 tasks done

Release proposal: standard v8 #564

feross opened this issue Jul 12, 2016 · 71 comments · Fixed by #603
Labels
Milestone

Comments

@feross
Copy link
Member

feross commented Jul 12, 2016

Planned release date: September 1, 2016. (approx. 45 days from the date of this issue)

New features

New rules

(Estimated % of affected standard users, based on test suite in parens)

Changed rules

Internal changes

@feross
Copy link
Member Author

feross commented Jul 13, 2016

I'm happy to announce the immediate availability of a standard v8 release candidate. The version is 8.0.0-beta.0. The final version will be released around September 1, 2016 to allow ample time for community feedback.

You try it out today by installing it: npm install -g standard@beta

Cheers! 🎉

@feross
Copy link
Member Author

feross commented Jul 22, 2016

Hey collaborators! I have a favor to ask: Can you test out the standard v8 beta on one or two of your repos?

npm install -g standard@beta

Feedback would be great!

@maxogden @mafintosh @othiym23 @dcposch @Flet @dcousens @jprichardson @xjamundx @watson @rstacruz @reggi @yoshuawuyts @bcomnes @jb55

@yoshuawuyts
Copy link
Contributor

Tested it on 4 or so (ES6 heavy) repos, all seems good 🎉

@feross
Copy link
Member Author

feross commented Jul 22, 2016

Thanks @yoshuawuyts!

@feross
Copy link
Member Author

feross commented Jul 23, 2016

standard 8.0.0-beta.1 is out, which just updates eslint from ~3.0.0 to ~3.1.0.

@jprichardson
Copy link
Member

jprichardson commented Jul 23, 2016

Just tried it on a very large project I'm working on, got this error:

standard         
standard: Unexpected linter output:

TypeError: Cannot read property 'value' of undefined
    at getStarToken (/Users/jp/.nvm/versions/node/v6.1.0/lib/node_modules/standard/node_modules/eslint/lib/rules/generator-star-spacing.js:68:25)
    at EventEmitter.checkFunction (/Users/jp/.nvm/versions/node/v6.1.0/lib/node_modules/standard/node_modules/eslint/lib/rules/generator-star-spacing.js:123:29)
    at emitOne (events.js:101:20)
    at EventEmitter.emit (events.js:188:7)
    at NodeEventGenerator.enterNode (/Users/jp/.nvm/versions/node/v6.1.0/lib/node_modules/standard/node_modules/eslint/lib/util/node-event-generator.js:40:22)
    at CodePathAnalyzer.enterNode (/Users/jp/.nvm/versions/node/v6.1.0/lib/node_modules/standard/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:607:23)
    at CommentEventGenerator.enterNode (/Users/jp/.nvm/versions/node/v6.1.0/lib/node_modules/standard/node_modules/eslint/lib/util/comment-event-generator.js:97:23)
    at Controller.traverser.traverse.enter (/Users/jp/.nvm/versions/node/v6.1.0/lib/node_modules/standard/node_modules/eslint/lib/eslint.js:902:36)
    at Controller.__execute (/Users/jp/.nvm/versions/node/v6.1.0/lib/node_modules/standard/node_modules/estraverse/estraverse.js:397:31)
    at Controller.traverse (/Users/jp/.nvm/versions/node/v6.1.0/lib/node_modules/standard/node_modules/estraverse/estraverse.js:501:28)

If you think this is a bug in `standard`, open an issue: https://github.com/feross/standard/issues

My config in package.json:

  "standard": {
    "ignore": [
      "static/"
    ],
    "parser": "babel-eslint",
  }

If I remove babel-eslint, it outputs expected standard errors, however then standard --fix barfs on decorators (legacy).

@feross
Copy link
Member Author

feross commented Jul 23, 2016

@jprichardson Thanks for testing.

This is an issue with the interaction between babel-eslint and eslint. See eslint/eslint#6274

I think we'll just have to disable the generator-star-spacing rule until ESLint has support for the async keyword. Bummer.

feross added a commit to standard/eslint-config-standard that referenced this issue Jul 23, 2016
There is an issue with the interaction between babel-eslint and eslint.
See eslint/eslint#6274

I think we'll just have to disable the generator-star-spacing rule
until ESLint has support for the async keyword. Bummer.

See standard/standard#564 (comment)
@feross
Copy link
Member Author

feross commented Jul 23, 2016

standard 8.0.0-beta.2 is out, which removes the generator-star-spacing rule so projects that use babel and have an async generator function will work.

@jprichardson Can you test it out now?

@jprichardson
Copy link
Member

Wow. standard --fix feels like magic compared to what I use to have to do - I figured it would take me forever to migrate. Haha. So far it looks like everything is working. Gotta fix a lot of pesky object-property-newline, but that's ok. At first glance, the React changes don't bother me like I thought that they might. I'll keep playing with it.

Nice work @feross!

@feross
Copy link
Member Author

feross commented Jul 23, 2016

@jprichardson Good to hear. Thanks for helping to test it out!

@Flet
Copy link
Member

Flet commented Jul 25, 2016

Thanks for kicking this off!

Any opposition to bumping to eslint-plugin-react@5.2.2?

There have been quite a few fixes and additions since 5.0.1:
https://github.com/yannickcr/eslint-plugin-react/blob/master/CHANGELOG.md

Seems like a good time to update?

@Flet
Copy link
Member

Flet commented Jul 25, 2016

Released semistandard@beta with updated standard config... its strangely satisfying to run --fix to toggle between standard and semistandard 😈

@feross
Copy link
Member Author

feross commented Jul 25, 2016

@Flet eslint-plugin-react@5.2.2 is already used because of the semver range we're using ^5.0.1. But I just pushed a commit that forces the latest version of all deps, just to be sure an older version is never used. f6b92de

@feross
Copy link
Member Author

feross commented Jul 25, 2016

Just merged one new rule: Require block comments to be balanced (#572)

Released standard 8.0.0-beta.3.

@timoxley
Copy link
Contributor

@feross 👍 👍 Tried out 8.0.0-beta.3. No dramas. The JSX changes looked like a real PITA since it wasn't going to be a simple regex/find/replace but standard --fix did all the hard work. LGTM ✔️

@feross
Copy link
Member Author

feross commented Jul 26, 2016

@timoxley 👍 👍 👍 great to hear! what do you think of the jsx change?

@timoxley
Copy link
Contributor

timoxley commented Jul 26, 2016

what do you think of the jsx change?

@feross I do understand & accept the sentiment in standard/eslint-config-standard#27, though it probably wouldn't be my preference.

  • One quoting rule is better than two quoting rules, especially within the one file. Find/replace for cleaning mixed js/jsx no longer trivial.
  • Single quotes are a little tidier. Fewer unnecessary pixels is better IMO.
  • Double quotes in html does make it easier to copy/paste html (though I did like the act of purifying html of its double quote filth).
  • Creates a bit of a reliance on --fix since layman's find/replace isn't js/jsx sensitive. This is probably fine so long as --fix doesn't go the way of standard-format i.e. writing broken/mangled code.
  • Since it's much of a muchness, I'm not sure the benefits outweigh the downsides of requiring sweeping changes across every codebase using JSX. Perhaps this is ok though, given that --fix exists & works.

@timoxley
Copy link
Contributor

timoxley commented Jul 26, 2016

@feross I think you could head-off many complaints if standard promoted the use of standard --fix whenever it finds issues. As a part of the tagline perhaps?

Created an issue for this: #576

Update: Message promoting --fix has been added https://github.com/Flet/standard-engine/pull/115/files 🎉

@kyeotic
Copy link

kyeotic commented Jul 26, 2016

I agree with @timoxley that fix should have more awareness. I manually updated all my code (though it was easier with "replace in selection" and only two JSX projects) because I didn't know about fix.

Maybe it could go into the error output?

@timoxley
Copy link
Contributor

@tyrsius see #576

@dcousens
Copy link
Member

dcousens commented Jul 27, 2016

I fully agree with #564 (comment), in that I'm not sure the benefits out weigh the extra rule.

@timoxley
Copy link
Contributor

timoxley commented Jul 30, 2016

So the JSX double quoting rule requires mixing of double/single quotes when doing any kind of inline JS in JSX:

<div
  doubleQuotes="just because this is static"
  butForDynamicValues={singleQuotes === 'are' ? 'needed' : 'now'}
/>

Or a more realistic example:

<div title="Help Text" className={selected === item.id ? 'selected' : 'unselected'}>
  {item.content}
</div>

This converted me to a mild 👎 about this change; needing to use two quoting styles on the same line is not very "don't make me think" IMO.

One rule > Two rules

@bentatum
Copy link

I'm not a fan of double quotes in jsx. One of the things that turned me on to standard was its simplicity. I think this is a step backward in that respect.

@timoxley
Copy link
Contributor

timoxley commented Aug 13, 2016

@feross Another option could be for standard itself to check the node version and error appropriately rather than just waiting for it to fail on some syntax error. Perhaps then could add a flag like --ignore-unsupported so standard always exits 0 for users who need to run standard as a part of the test suite on older nodes.

For reference, this is the current error when running the latest standard beta (8.0.0-beta.4) on node 0.10.46:

> standard

/usr/local/lib/node_modules/standard/node_modules/eslint/node_modules/strip-bom/index.js:2
module.exports = x => {
                    ^
SyntaxError: Unexpected token >
    at Module._compile (module.js:439:25)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> (/usr/local/lib/node_modules/standard/node_modules/eslint/lib/config/config-file.js:23:16)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)

@jprichardson
Copy link
Member

Standard 8 is gonna land soon. We gotta figure out the JSX double quotes situation...

Given that all production versions of Standard before this don't allow JSX double quotes AND we don't have consensus, it should be removed from the Standard v8 release. Not too mention though other reasons like 1) Facebook themselves are inconsistent 2) it's confusing for new users (different rules (when/where) 3) JSX is not HTML. 4) --fix makes it crazy easy to copy and paste HTML.

@kyeotic
Copy link

kyeotic commented Aug 16, 2016

@jprichardson

  1. They have been consistent for a while now, examples of inconsistency are old
  2. JSX Is not JavaScript either
  3. --fix also makes it crazy easy to fix all the production versions

You might disagree with the decision, but saying we have to "figure it out" ignores the multiple threads over which this has already been discussed. The fact that its in the release is it being "figured out".

@dcousens
Copy link
Member

@tyrsius I think what @jprichardson is saying, is perhaps the final decision should be delayed rather than delaying v8.

@pluma
Copy link

pluma commented Aug 17, 2016

@jprichardson the logical conclusion if there is no consensus isn't to enforce your taste but to remove the validation until there is a consensus. JSX is neither HTML nor JS so I'm not even sure it belongs in standard at all. In fact, your entire "JSX isn't HTML" argument is already violated by the enforcement of boolean properties being passed with the shorthand syntax (i.e. <MyComponent booleanProp /> rather than <MyComponent booleanProp={true} />) and the enforcement of the XHTML-style space before the closing slash in empty components.

That said, there was a consensus. Just one you happen to disagree with.

@timoxley
Copy link
Contributor

That said, there was a consensus. Just one you happen to disagree with.

There's also some consensus that the current consensus is disagreeable.

@pluma
Copy link

pluma commented Aug 17, 2016

@timoxley which indicates this really shouldn't be part of standard to begin with.

@joshmanders
Copy link

Agreed with @pluma. It should just be removed. I really enjoy using standard but I can't stand single quotes in my html, OCD about it, and whether you see it as html or not, JSX to me is html-like, so that OCD triggers there. I'd hate to have to deviate away from the ease of just using standard all because of this.

@LinusU
Copy link
Member

LinusU commented Aug 17, 2016

I don't think it should be removed. Standard is all about just picking one way and sticking with it. I don't see any great advantage with either one, but having it not enforce any of them would be worse.

My vote goes to @feross just picking one of them. It's not like it going to affect anyones day to day life. Both ways are easy to fix with the new --fix flag.

@feross
Copy link
Member Author

feross commented Aug 18, 2016

@timoxley Another option could be for standard itself to check the node version and error appropriately rather than just waiting for it to fail on some syntax error

Good idea. Let's skip running standard on Node <4 and print a warning to stderr. I also added an install-time warning using the package.json "engines" field to warn on Node <4.

PR here: #590

@feross
Copy link
Member Author

feross commented Aug 18, 2016

@dcousens This doesn't look like it has an easy answer @feross, I think we need a decision?

Yeah, tell me about it. Either decision we make, there are going to be some folks who aren't happy. That's just the way a curated style guide like this works.

There's no clear winner, IMO. Quoting some of the arguments from this thread, here's how I would sum this up:

Pros for JSX double quotes:

  • Makes copy/pasting from HTML examples easier.
  • Matches the majority of the React/HTML community's style.
  • Makes JSX "look like HTML" more than when single quotes are used.
  • Forces the developer to understand what is JSX and what is JS.
  • Avoids escaping in attributes that contain an apostrophe (e.g. <img alt="I'm happy">

Pros for JSX single quotes:

  • "Always use single quotes" is easier to remember. Less cognitive overhead.
  • Friendlier to beginners. Doesn't require mixing double/single quotes when inline JS is used in JSX.
  • Single quote attributes are totally valid in HTML.
  • Arguably tidier. Fewer unnecessary pixels on screen.
  • Avoids huge "upgrade to standard v8" commit in existing codebases using standard v7.
  • Easier to find/replace double quotes with single using text editor tools, whereas to switch to double requires requires reliance on --fix.

The standard philosophy is to enforce consistency to maximize clarity and reduce programmer error. The decision to switch to double quotes in JSX was made to increase clarity (so it looks more like HTML and distinguishes it from the surrounding JS).

But in attempting to switch some of my own code to the new v8 style, I'm running into situations where the intermixing of single and double quotes feels like it's actually reducing clarity. This is a typical example:

<img className="poster" alt={this.getAltText('poster')} />

In this situation, the rule is not reducing the likelihood of errors or increasing the clarity of the code. There's added cognitive overhead to remember when to use each type of quote.

I worry that beginners won't be able to figure out when to use each type of quote, because we've taken a simple rule ("always use single quotes") and made it more complex ("use single quotes for JS, use double quotes for JSX").

There's no perfect decision here. But I think we should revert to standard v7 behavior ("always use single quotes") since that's the simpler choice.

Note that allowing any type of quote isn't acceptable. Whenever there's two ways to do something without a clear winner, standard tries to "just pick something".

@feross feross modified the milestone: standard v8 Aug 19, 2016
feross added a commit to standard/eslint-config-standard-jsx that referenced this issue Aug 19, 2016
@feross
Copy link
Member Author

feross commented Aug 19, 2016

Just published standard v8.0.0-beta.5 with the following 4 new rules:

And, changed rule:

  • Changed the jsx-quotes rule to enforce single quotes.

I promoted this release to the "latest" npm dist-tag so it gets installed with npm install standard, to increase exposure. The "beta" dist-tag is no more.

If there are no issues, this release will become the official standard v8.0.0 release on September 1, 2016.

@feross
Copy link
Member Author

feross commented Aug 22, 2016

I just realized that my trip to China on August 26 might interfere with the planned September 1 release date.

So, new release date: August 23 (one week early).

That way I have a couple days to fix any issues that might come up. Also, there are plenty of contributors who have github/npm permission who can step up if some urgent issue surfaces while I'm gone.

feross added a commit that referenced this issue Aug 24, 2016
@feross
Copy link
Member Author

feross commented Aug 24, 2016

standard v8.0.0 is released!

standard v8

@Jessidhia
Copy link

@feross you probably should make a non-beta release of eslint-config-standard (and depend on that instead) :)

@feross
Copy link
Member Author

feross commented Aug 24, 2016

@Kovensky good call, done

@robinpokorny
Copy link

@feross Should eslint-config-standard-react v4 be released with eslint-config-standard-jsx@3.0.0 dependency?

@feross
Copy link
Member Author

feross commented Aug 26, 2016

@robinpokorny Done

gemmaleigh added a commit to alphagov/govuk_elements that referenced this issue Oct 11, 2016
https://github.com/feross/standard/blob/master/CHANGELOG.md

8.4.0 - 2016-10-10

Update ESLint from 3.6.x to 3.7.x.
5 additional rules are now fixable with standard --fix!
Use more conservative semver ranges #654
8.3.0 - 2016-09-29

The last release (8.2.0) added ES7 support. This release (8.3.0) adds
ES8 support ...just 3 days later!

This release should eliminate the need to specify babel-eslint as a
custom parser, since standard can now parse ES8 (i.e. ES2017) syntax
out of the box. That means async and await will just work.

Support ES8 (i.e. ES2017) syntax.
8.2.0 - 2016-09-26

For many users, this release should eliminate the need to specify
babel-eslint as a custom parser, since standard can now parse ES7 (i.e.
ES2016) syntax out of the box.

Support ES7 (i.e. ES2016) syntax.
Update ESLint from 3.5.x to 3.6.x.
4 additional rules are now fixable with standard --fix!
8.1.0 - 2016-09-17

Update ESLint from 3.3.x to 3.5.x.
Around 10 additional rules are now fixable with standard --fix!
8.0.0 - 2016-08-23

This release contains a bunch of goodies, including new rules that
catch potential programmer errors (i.e. bugs) and enforce additional
code consistency.

However, the best feature is surely the new --fix command line flag to
automatically fix problems. If you ever used standard-format and ran
into issues with the lack of ES2015+ support, you'll be happy about
--fix.

standard --fix is built into standard v8.0.0 for maximum convenience,
it supports ES2015, and it's lightweight (no additional dependencies
since it's part of ESLint which powers standard). Lots of problems are
already fixable, and more are getting added with each ESLint release.

standard also outputs a message ("Run standard --fix to automatically
fix some problems.") when it detects problems that can be fixed
automatically so you can save time!

With standard v8.0.0, we are also dropping support for Node.js versions
prior to v4. Node.js 0.10 and 0.12 are in maintenance mode and will be
unsupported at the end of 2016. Node.js 4 is the current LTS version.
If you are using an older version of Node.js, we recommend upgrading to
at least Node.js 4 as soon as possible. If you are unable to upgrade to
Node.js 4 or higher, then we recommend continuing to use standard v7.x
until you are ready to upgrade Node.js.

Important: We will not be updating the standard v7.x versions going
forward. All bug fixes and enhancements will land in standard v8.x.

Full changelog below. Cheers!

New features

Upgrade to ESLint v3
(http://eslint.org/docs/user-guide/migrating-to-3.0.0) #565
BREAKING: Drop support for node < 4 (this was a decision made by the
ESLint team)
Expose ESLint's --fix command line flag #540 standard-engine/#107
Lightweight, no additional dependencies, fixes dozens of rules
automatically
New rules

(Estimated % of affected standard users, based on test suite in parens)

Enforce placing object properties on separate lines
(object-property-newline) #524 (2%)
Require block comments to be balanced (spaced-comment "balanced") #572
(2%)
Disallow constant expressions in conditions (no-constant-condition)
#563 (1%)
Disallow renaming import, export, and destructured assignments to the
same name (no-useless-rename) #537 (0%)
Disallow spacing between rest and spread operators and their
expressions (rest-spread-spacing) #567 (0%)
Disallow the Unicode Byte Order Mark (BOM) (unicode-bom) #538 (0%)
Disallow assignment to native objects/global variables
(no-global-assign) #596 (0%)
Disallow negating the left operand of relational operators
(no-unsafe-negation) #595 (0%)
Disallow template literal placeholder syntax in regular strings
(no-template-curly-in-string) #594 (0%)
Disallow tabs in file (no-tabs) #593 (0%)
Changed rules

Relax rule: Allow template literal strings (backtick strings) to avoid
escaping
 #421
Relax rule: Do not enforce spacing around * in generator functions
(standard/standard#564 (comment))
This is a temporary workaround for babel users who use async generator
functions.
@yantakus
Copy link

@feross You forgot to mention one Pros for JSX double quotes and it is the most important of all: JSX is treated as HTML in most editors, so they use double quotes by default. I mean when you start typing an attribute and then choose from autocomplete options. Editors insert double quotes in this case. And this behavior is even not configurable for some editors, eg. Intellij Idea. It is possible to disable auto-insertion of quotes at all, but not auto-insert single quotes: http://stackoverflow.com/questions/37635871/how-can-i-disable-syntax-autocomplete-of-jsx-properties.

At this moment this is the only rule in Standard that forces me to overwrite rules. Kinda frustrating...

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.