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

RFC: Migration to ESLint #1913

Closed
amilajack opened this issue Dec 31, 2016 · 19 comments
Closed

RFC: Migration to ESLint #1913

amilajack opened this issue Dec 31, 2016 · 19 comments

Comments

@amilajack
Copy link
Contributor

ESLint has gained much traction in the JS community. Would a migration to eslint be welcome?

@roblarsen
Copy link
Member

Anyone? I'm content to leave the status quo. ESLint is great, but I'd need to be convinced to make the change (and then someone would have to make the change in a PR.) I'll leave this open for another week and if there's no action I'll close.

@amilajack
Copy link
Contributor Author

amilajack commented Mar 18, 2017

I can PR for this. ESLint is arguably one of the most widely used tools in the javascript ecosystem.

http://www.npmtrends.com/eslint-vs-jshint

screen shot 2017-03-18 at 11 04 30 am

@AlexxNica
Copy link

AlexxNica commented Mar 19, 2017

@roblarsen I'm totally at @amilajack's side on this,
I made a brief comparison between the two with some data I gathered from the Web if you wanna to take a look. It's a gist, click me

@roblarsen
Copy link
Member

Thanks guys. That's great stuff. Anyone else want to chime in?

@amilajack thanks for the offer to PR. It's really appreciated.

@roblarsen roblarsen reopened this Mar 19, 2017
@AlexxNica
Copy link

AlexxNica commented Mar 19, 2017

@roblarsen You're welcome!
@amilajack Feel free to message me if you come to need any help. 👷

@ryzokuken
Copy link

@amilajack Are you still working on this?

@roblarsen
Copy link
Member

@ryzokuken There's an open PR that I need to take a look at and merge in. #1930

@ArmorDarks
Copy link

I'd recommend to look into Standard which is addition on top of ESLint, and quite popular one. To be honest, it as relief for use to finally stop maintaining ESLint configs...

@AlexxNica
Copy link

@ArmorDarks Even though I like the Standard style, I don't think it's good to enforce others to follow it. Especially developers without much knowledge or experience, as their code may lead to unexpected runtime errors and cause further problems, even leading to increased number of issues for the project.

As much as I like programming language's flexibility, such perk comes at a price, and the question is if we're willing to pay that price. Stricter rules almost always make the code easier to understand and more reliable as a consequence, even as the project grows.

Moreover, semicolons win by popularity, taking Airbnb's JavaScript style guide as an example: one of the most starred repositories on GitHub and higher download numbers on NPM.[1][2][3][4]

Also, we would need to update all JavaScript files, as they're all using semicolons already.[5][6]

Therefore, I don't believe this subject worth the bikeshedding.

References:

[1] https://github.com/airbnb/javascript
[2] https://www.npmjs.com/package/eslint-config-airbnb-base
[3] https://www.npmjs.com/package/eslint-config-airbnb
[4] https://www.npmjs.com/package/eslint-config-standard
[5] https://github.com/h5bp/html5-boilerplate/blob/master/gulpfile.babel.js
[6] https://github.com/h5bp/html5-boilerplate/blob/master/src/js/plugins.js

@ArmorDarks
Copy link

That all will boil down to codestyle disputes anyway. I'm just proposing battle-tested alternative.

Btw, I should highlight, that no matter with what option boilerplate will go (be it ESLint with project's own rules, be it popular ESLint config, like airbnb, or be it Standard), it will anyway enforce something on users to follow. So it's just a matter of what exactly will be enforced.

@roblarsen
Copy link
Member

At this point I want to switch to eslint with the least possible interruption. That includes blowing away the existing rules we have in place wholesale without at least a discussion of what new rules should be (which is why the existing PR for eslint is DOA.)

I will die on the hill of semicolons, for example.

As a note: the actual consumers of the ruleset is limited to the maintainers and anyone who is conscientiously creating PRs and actually testing their code. We won't ship an eslintrc in the dist. The real benefit, in terms of the project, is that we do it and can serve as an example of what good web dev should look like.

@ArmorDarks
Copy link

ArmorDarks commented Jul 26, 2017

I will die on the hill of semicolons, for example.

That's just few strokes with regex, you know. But whether use them or no for some people indeed seems to be painful theme...

We won't ship an eslintrc in the dist. The real benefit, in terms of the project, is that we do it and can serve as an example of what good web dev should look like.

But you ship the JS code, which will be included in other projects, which will later be linted, and people will be forced or to edit it to fit their rules, or to use HTML5Boilerplate rules. That's why JS should be as close to JS community standards as possible, and I think that making some necessary changes to rules and codestyle is unavoidable. And not that problematic, really — Standard and ESLint with --fix will automatically fix most part of introduced rule error.

The real issue is what to consider community standard in JS world.

@roblarsen
Copy link
Member

Good points. It's good to be reminded of the downstream effects of even the small amount of JS we do ship.

And to clarify I'm not against making changes. What I don't want to do is make wholesale changes without discussion (so thanks for being part of the discussion 👍 ) While I'm rather desperate to get 6.0 out the door right now, I'm generally in no rush to make changes here unless they're the right changes.

@TheDancingCode
Copy link
Contributor

One option is to apply the eslint:recommended config which reports common problems, but doesn't really impose stylistic preferences. In that case, ESLint would just serve to prevent errors and bugs. It really is a kind of minimal, common sense config.

The only style-related rules I would propose to add in that case are the ones present in your .editorconfig: no trailing whitespace, 4 space indents, and final newlines.

@ArmorDarks
Copy link

4 space indents

Just for the information, none of the popular code styles using 4 spaces indents anymore.

@millette
Copy link

#1913 (comment) (eslint:recommend) sounds good, although I have a personal preference for #1913 (comment) (StandardJS).

Jokingly: https://github.com/christophwitzko/semicolon-indent#semicolon-indent

@ArmorDarks
Copy link

I'm with Standard too, because they not only maintain good config with validation through the community but also shipping excellent tested additions on top of the ESLint.

But just to be fair, I should also mention Prettier. Though, my vote still stands for Standard.

@roblarsen
Copy link
Member

Reminder from earlier in the discussion

I will die on the hill of semicolons, for example.

@ArmorDarks
Copy link

Imagine that it's surviving game, and like we're in the 2005 year deciding on tabs vs spaces.

Btw, worth mention the thing about Prettier — it should be paired with good ESLint config, because mostly all Prettier maintains is code style, while ESLint configs (and especially Standard's one) covering also a lot of non-stylistic good or bad practices.

roblarsen added a commit that referenced this issue Mar 31, 2018
roblarsen added a commit that referenced this issue Apr 6, 2018
Fixes #1913

* Drops node 4

* update node engine to >=6 (#2035)

* syncs changes to spacing across different editorconfigs

* adds eslint with minimally invasive configuration- basically moving from 4 to 2 spaces.
kaypeter87 added a commit to kaypeter87/html5-boilerplate that referenced this issue Sep 1, 2019
* Restores stable visuallyhidden (h5bp#1991)

see the saga unfold #1985

* adding npm instructions

* Fixed JSHint errors (h5bp#1994)

* What about the meta tag, shrink-to-fit=no (h5bp#1984)

https://stackoverflow.com/questions/33767533/what-does-the-shrink-to-fit-viewport-meta-attribute-do
It worth?
I am trying to help please don't blame me

* double word on line 21 (be) (h5bp#1996)

Please the word 'be' duplicated and corrected that. 
Thanks.

* Add documentation for humans.txt (h5bp#2007)

* Add documentation for humans.txt

* Change src instead of dist

* Update `.htaccess` to v2.15.0 from h5bp/server-configs-apache (h5bp#2003)

* Update extend.md (h5bp#2006)

* Update extend.md

* Update extend.md

* Create CODE_OF_CONDUCT.md (h5bp#2011)

* Updated tests (Travis and Mocha) and dependencies, rebuilt dist (h5bp#2009)

* docs: remove trailing whitespace in CHANGELOG.md

* test: update node versions & add trusty to travis

* test: update deps and test script in package.json

* refactor: rebuild & add updated docs, css, & html

* docs: remove duplicate 'be' in usage.md

* Upgrading to babel-preset-env (h5bp#2010)

* Remove info about Flash in Metro mode IE10 (no longer applicable) (h5bp#2015)

This was addressed nearly 5 years ago:
https://blogs.msdn.microsoft.com/ie/2013/03/11/flash-in-windows-8/

* Update to jQuery 3.3.0 (h5bp#2016)

* update dev dependencies (h5bp#2017)

* Update to jQuery 3.3.1 (h5bp#2018)

* Minor CSS comments update (h5bp#2019)

* update link to Atom RSS (h5bp#2024)

* Remove `clip-path` (h5bp#2025)

Revert h5bp#1920 to fix h5bp#2021.

* Devdeps update (mocha, modernizr, normalize and gulp-autoprefixer) (h5bp#2028)

* Update Dev Dependencies

mocha, modernizr, gulp-autoprefixer updates.
The build tests run fine with no changes. The dist CSS is different as the PR from 2025 didn't include changes in the dist dir.

* Update Normalize

* Update JS head snippets (h5bp#2029)

plus correct capitalization of JavaScript

* Adding a note about PRs

* Add .babelrc and .prettierrc to .gitattributes (h5bp#2030)

* Replace `node-sri` with `ssri` (h5bp#2031)

Closes h5bp#1997.

* update devdeps (h5bp#2032)

* Update package.json (h5bp#2034)

* HTTPS links (h5bp#2036)

* https a link

* https more links

* Fix paramaters typo

* Remove trailing . (unneeded)

* getting indent lined up across the project

* Features/eslint (h5bp#2037)

Fixes h5bp#1913

* Drops node 4

* update node engine to >=6 (h5bp#2035)

* syncs changes to spacing across different editorconfigs

* adds eslint with minimally invasive configuration- basically moving from 4 to 2 spaces.

* Bump Apache Server Config to v3 (h5bp#2042)

* Update CHANGELOG.md

* adds 2037

* adds 2042

* Prepare 6.1 release (h5bp#2045)

- Update `CHANGELOG.md` date
- Update dev deps
- Indenting on `index.html` to 2 space (was 4 space)

* change the attribute of "notranslate" to "content" (h5bp#2050)

* change the key of "notranslate" to "content"

"value" is not a valid attribute on element meta (http://w3c.github.io/html/single-page.html#the-meta-element).
According to https://support.google.com/webmasters/answer/79812?hl=en,
content="notranslate" is recommended to use.

* don't specify the language on the Google site

* Update dev deps (h5bp#2053)

updates to:
```
babel-preset-env
gulp-rename
gulp-replace
mocha
```

* https links to editorconfig (h5bp#2055)

* Ignore package-lock.json

We already do this on the html5boilerplate.com site
REF: https://github.com/h5bp/html5boilerplate.com/blob/master/.gitignore

* removes package-lock.json

* Update dev deps (h5bp#2058)

* Update dev deps

* Update package.json

update eslint again

* Update file_existence.js

* Update TOC.md

* Improve manifest

* Reset incorrect site manifest

* Update correct site manifest and apply suggestions

* updates to dev deps (h5bp#2061)

* Update src manifest

* Update dist manifest

* update links (h5bp#2062)

- Fix for link to Google for mobile seperate links
- https link to hixie.ch
- fix link for Twitter Cards for Developers

* Add Install with Yarn option (h5bp#2063)

Yarn is popular these days and H5BP is currently getting 2.4K downloads per month so I think it'd be good to add this here.
I changed the list of options to unnumbered as they aren't steps/instructions so bullets make more sense imho.

* bump eslint version to 5.6.0

* Fixing links on the EXTEND page (h5bp#2065)

- Removed dead link to http://dayofjs.com/videos/22158462/web-browsers_alex-russel
- Fix many redirects to browser docs on Mozilla, Apple, Faceook and Microsoft sites

* v7.0 (WIP)

- Remove unneeded x-ua-compatible - not needed for IE11
- Update many external links
- Fix typos
- Add info for creating a good meta description
- Add link for iPhone X viewport info
- Update browser upgrade prompt
- Remove integrating Bootstrap with H5BP (it was written a longtime ago and I don't think is relevant so much)
- Remove statement that Twitter Cards requires registering a domain
- Remove official shortlink section, it was always very poorly supported
- Made the touch icons sections more consise (no need at all to mention about non-retina iPhones and iOS versions prior to iOS 6 etc)

* update README to links to previous versions

- update package,json deps

* Re Add display standalone attribute

* Re Add display standalone attribute

* Add <meta name="theme-color" content="#fafafa">

* Add security.txt details

* copies main.css from main.css project (h5bp#2066)

This code updates the package.json, build and tests to pull main.css from the main.css npm package.

* Removes out of date comments (h5bp#2077)

Fixes h5bp#2052

* removing travis-after-all (h5bp#2078)

closes h5bp#2020

* updating CSS docs to reference main.css (h5bp#2079)

Also, o need to dupe this documentation over two repos.

* Removes "display": "standalone" from manifest (h5bp#2096)

* Removed link to #404html (h5bp#2099)

I've removed link to #404html section of the page, since there is on section for 404 page and in IDE I'm getting "Cannot resolve anchor #404html" warning. I hope that it makes sense.

* Upgrade Normalize.css from 8.0.0 to 8.0.1 (h5bp#2104)

* Upgrade Normalize.css from 8.0.0 to 8.0.1

* Update Normalizecss version

* remove instances of shrink-to-fit=no (h5bp#2103)

closes h5bp#2102

Per my findings, the need for it as a default was rectified with the release of iOS 9.3, where the viewport no longer shrunk to accommodate overflow, as was introduced in iOS 9.

* Update CHANGELOG.md

* Update package.json

* the CSS and html MD files hadn't been updated

* Bump apache-server-configs to v3.1.0

* Update .htaccess with config v3.1.0

* updating changelog

* Typo

* npm published

* 7.0.1 (h5bp#2112)

updates main.css to latest, updates changelog and bumps version

* Update index.html

A more succinct way of writing the conditional comment -- still recognized by IE 5-9. (IE10+ doesn't support conditional comments)

* Update index.html

* Minor update to eslint-plugin-mocha and eslint (h5bp#2114)

* minor docs update (h5bp#2115)

Update https://cdn.polyfill.io from v2 to v3
Remove unneeded shrink-to-fit=no meta tag

* Update Modernizr to 3.7.0 (h5bp#2116)

* update CSS from main.css in dist (h5bp#2119)

* Update Google Analytics docs and snippet (h5bp#2118)

- We now include ` ga('set','transport','beacon');` in the `index.html` analytics snippet for improved peformance
- Docs updated mentioning why we are using 'analytics.js' rather than 'gtag.js'. Removed link to mathias bynens old blog post as its not so relevant since the async snippet is included in the official Google Develoepr docs. There's a link to Philip Walton's excellent guide to advanced setup.

* Update modernizr-3.7.0.min.js (h5bp#2120)

This wasn't done correctly in the previous PR.

* Upgrading modernizr and bumping version

* update version to v7.1.0

+ minor devdeps update
+ update CHANGELOG.md

* remove unnessecary info

* revert removal of paragraph and instead clarify 'serve jQuery to users in China'

* update jQuery to v3.4.0

Improved performance and a minor vulnerability fix

REF: http://blog.jquery.com/2019/04/10/jquery-3-4-0-released/

* update jQuery and Modernizr

* Update to jQuery 3.4.1

+ minor devdeps update

* update Apache Server Configs to 3.2.1

* Remove defer from Google Analytics snippet

* Update CHANGELOG.md

* updating main.css

* Add License, NPM Download counter and GitHub Stars counter badges

* added main.css change

* fix broken links in TOC

* update version number to v7.2.0

* gulp-load-plugins update

fix a security vuln https://github.com/jackfranklin/gulp-load-plugins#changelog

* Revert "gulp-load-plugins update"

This reverts commit ec96be3.

* update version number in main.css

+ security vuln devdep update

* Add docs update PRs to CHANGELOG

* bump lowest supported version of node to 8.x

previous versions are end-of-life.

REF: https://github.com/nodejs/Release

* Removed package-lock.json from .gitignore

Removed package-lock.json from .gitignore

package-lock.json is intended to be checked into source control, without a package-lock.json using npm makes little sense over yarn. 

I see this was added in 2017, when there was an issue with the package-lock.json updating, this issue is now long resolved.

* adds package-lock.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants