Skip to content
This repository was archived by the owner on Aug 26, 2021. It is now read-only.

[WIP] eslint code hinting #700

Closed
wants to merge 1 commit into from
Closed

[WIP] eslint code hinting #700

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 26, 2019

I've tried to configure eslint rules, based on scripts/helper.js. Can use this for more accurate further settings.

@ghost ghost changed the base branch from master to develop March 26, 2019 18:11
@azerella azerella added submitted by user Issues on behalf of users. enhancement New feature or request. labels Mar 26, 2019
@azerella
Copy link
Contributor

azerella commented Mar 26, 2019

Thanks @toxamiz! I'll review the linting rules over the next few days, looks like a really good start though. We should also be looking at tools like https://www.npmjs.com/package/husky for when commits are made and ideally everything is formatted automatically.

I'm a bit skeptical on whether eslint supports some of the rules that we use, if it does that's great but we'll still need a way to automatically format them

@ghost
Copy link
Author

ghost commented Mar 27, 2019

For first look there are some indent issues (or the base file have indent issues :) ) and there no base rule for UpperCamesCase for global functions const CreateDir = ( dir ) => {...}.
Eslint can be extended with custom rules, if there are few missing rules, we can try to make them (or try to find)

@azerella
Copy link
Contributor

Looks good @toxamiz, some more feedback:

  • Rename eslintconfigs/ to .eslint/
  • Add a lint script for jQuery, something like: "lint:js": "eslint -c .eslint/jquery.json packages/*/src/js/jquery.js"

We need to make a conscious effort that the module.js and jquery.js files are IE8+ compliant.

Looking at a plugin like: https://www.npmjs.com/package/eslint-plugin-ie11 might be a suitable option, or at least inheriting some of the rules from this.

'browser': true
},
'parserOptions': {
'ecmaVersion': 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'ecmaVersion': 5
'ecmaVersion': 5

I'm not confident this supports IE8-11 rules

Copy link
Author

Choose a reason for hiding this comment

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

It just forbid to use es6 syntax e.g. let, const, etc

@azerella azerella changed the title eslint config draft [WIP] eslint code hinting Mar 29, 2019
@ghost
Copy link
Author

ghost commented Mar 29, 2019

https://www.npmjs.com/package/eslint-plugin-ie11 might be a suitable option, or at least inheriting some of the rules from this.

I don't think it is what we need

I tried to use https://github.com/amilajack/eslint-plugin-compat which should lint the browser compatibility, but i doesn't work properly: allowing to use [].map on ie8 for example.

Btw, don't you think about to just require module.js and jquery.js to be written only with es5 features (which is supported by ie9+) or use babel transpile, and advise your component users to include es5-shim and DOM-shim if they need support of ie8?.

With es5 requirement we can use 'env': {'es6': false}, 'parserOptions': {'ecmaVersion': 5}

@azerella
Copy link
Contributor

azerella commented Apr 1, 2019

Closing this in favour of #710. We'll move this discussion over to #710 @toxamiz

@azerella azerella closed this Apr 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request. submitted by user Issues on behalf of users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant