Tradeshift / eslint-config-tradeshift

ESLint Shareable Config for JavaScript Tradeshift Style
ISC License
3 stars 3 forks source link

Globals as opt-in? #7

Closed wejendorp closed 7 years ago

wejendorp commented 7 years ago

Maybe we can create a nested config, specifically for V4-apps / frontend. I just had a big mistake slip because of the server global being allowed (but not existing in scope).

@sqren @sampi thoughts?

sorenlouv commented 7 years ago

These are the current globals:

"globals": {
    "t": true,
    "ts": true,
    "gui": true,
    "app": true,
    "__app": true,
    "__config": true,
    "runtime": true,
    "server": true
}

They are specific to Apps/Apps-Server so should not be in the shared config. Eg. Collaboration-Backend doesn't have any of those globals, and will never have.

The same with env:

"env": {
    "es6": true,
    "browser": true,
    "node": true,
    "jasmine": true
}

We can not assume that every project will be a browser + node project using jasmine. It could be a node-only project using Mocha.

My suggestion: remove the env and globals from the shared config, and let the project specific configs deal with these.

wejendorp commented 7 years ago

I totally agree. Let's strip it down so it's only about style. Not .. implementation details.

sampi commented 7 years ago

sounds good to me, wanna make a PR? I see this https://github.com/Tradeshift/eslint-config-tradeshift/pull/9 but I don't see the actual changes inside of it

sorenlouv commented 7 years ago

The actual change was in https://github.com/Tradeshift/eslint-config-tradeshift/pull/8

wejendorp commented 7 years ago

8 has been merged and apps/apps-server have been migrated.

9 has been pushed to npm but is borked.