Add eslint

[bagage]

Closes #188

There is a bunch of errors yet, not sure what to do with them for now: "polyfills": ["URLSearchParams", "Symbol.iterator", "Promise", "fetch", "Math", "Number", "navigator"]

[nrenner]

Does the polyfills do anything or just suppress the errors for now (which would be Ok for me)?

I think we include polyfills for URLSearchParams, Promise and fetch. We would need to later check the others what to do.

[bagage]

Yes for now it just removes the errors. I think we need to fix some of the errors. Here is the full list:

yarn run v1.19.0
$ eslint .

/home/brouter/brouter-web/config.js
  3:18  error  URLSearchParams is not supported in Safari 7, Opera Mini all, iOS Safari 7.0-7.1, IE Mobile 11, IE 10, Baidu 7.12, Android Browser 4.1, QQ Browser 1.2  compat/compat

/home/brouter/brouter-web/config.template.js
  4:18  error  URLSearchParams is not supported in Safari 7, Opera Mini all, iOS Safari 7.0-7.1, IE Mobile 11, IE 10, Baidu 7.12, Android Browser 4.1, QQ Browser 1.2  compat/compat

/home/brouter/brouter-web/dist/url-search-params.js
  2:387   error  Symbol.iterator() is not supported in Safari 7, IE 10  compat/compat
  2:1238  error  Symbol.iterator() is not supported in Safari 7, IE 10  compat/compat
  2:1435  error  Symbol.iterator() is not supported in Safari 7, IE 10  compat/compat
  2:1639  error  Symbol.iterator() is not supported in Safari 7, IE 10  compat/compat
  2:1687  error  Symbol.iterator() is not supported in Safari 7, IE 10  compat/compat

/home/brouter/brouter-web/js/Browser.js
  6:26  error  navigator.maxTouchPoints() is not supported in Safari 7, IE 10  compat/compat

/home/brouter/brouter-web/js/control/TrackStats.js
  26:17  error  Math.trunc() is not supported in Safari 7, IE 10  compat/compat
  26:67  error  Math.trunc() is not supported in Safari 7, IE 10  compat/compat

/home/brouter/brouter-web/js/plugin/NogoAreas.js
  136:30  error  fetch is not supported in Safari 7, Opera Mini all, iOS Safari 7.0-7.1, IE Mobile 11, IE 10, Android Browser 4.1    compat/compat
  140:30  error  Promise is not supported in Safari 7, Opera Mini all, iOS Safari 7.0-7.1, IE Mobile 11, IE 10, Android Browser 4.1  compat/compat

/home/brouter/brouter-web/js/router/BRouter.js
  322:31  error  Number.parseFloat() is not supported in Safari 7, IE 10  compat/compat
  323:31  error  Number.parseFloat() is not supported in Safari 7, IE 10  compat/compat
  328:34  error  Number.parseFloat() is not supported in Safari 7, IE 10  compat/compat
  375:31  error  Number.parseFloat() is not supported in Safari 7, IE 10  compat/compat
  376:31  error  Number.parseFloat() is not supported in Safari 7, IE 10  compat/compat
  381:34  error  Number.parseFloat() is not supported in Safari 7, IE 10  compat/compat

✖ 18 problems (18 errors, 0 warnings)

info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[bagage]

@nrenner the only polyfill I found was promise. Where the others are coming from?

one way could be to use https://polyfill.io/v3/url-builder/ like https://polyfill.io/v3/polyfill.min.js?features=Number.parseFloat%2CMath.trunc%2CSymbol.iterator

[nrenner]

Fixing could mean either to polyfill/... or to restrict supported browsers, e.g. I think we could drop IE 10 (0.04% usage according to Can I use...).

The big question probably is if IE 11 (2.04%) should still be supported. To my surprise I noticed some time ago, that we currently don't (because of a few errors).

I added the polyfills manually and individually:

• URLSearchParams
  • seems npm url-search-params (WebReflection/url-search-params) has been deprecated and recommends @ungap/url-search-params now
  • separately copied over to dist in scripts_config task and included in index.html before config.js
  • wasn't aware that this polyfill itself needs polyfilling, would consider this a bug
  • quick-fix could be to comment out in config.js because its only use is deactivated by default (but also have it in mind for Letting user overload the BR.conf.host #252)
• fetch
  • fetch polyfill is npm whatwg-fetch (commit 2257ac0)
  • included automatically because of main property in its package.json

[bagage]

I think it would make sense to migrate to webpack so that it can take care of polyfills for us.

In the mean time, I believe we are good for half of them:

• URLSearchParams
• Symbol.iterator
• Promise
• fetch
• Math
• Number
• navigator
• flat() usage (polyfill)

[bagage]

So I've tested a bit current master and:

• it doesn't work on Edge 38 on Win10 because of unsupported flat() method
• it does't work on IE 11 on Win7 because of url-search-params (test here) because of URL.propotytpe being undefined. If I remove its usage from config.js and import from html, I encounter first issue again. I believe we can use that for now.

I believe we have to migrate to babel/webpack to ensure compatibility in the future.

[bagage]

I continued but there are actually errors in third parties code too (Object.assign cannot be used in IE10). I believe brouter stopped working on IE10 long time ago (maybe since the migration to bootstrap). Not sure what to do there, but I believe the best move would be to go for ES6 support and webpack + babel, I'm not sure this MR should be merged right now.

[nrenner]

I like that is shows where we stand and that it's already a bit of a mess. It will at least help to preserve the current state and not make it worse for now.

Webpack is the goal (I suppose), but I hope to get a release ready soon and I wouldn't want to introduce Webpack before that.

I also think that IE 11 has not been working for quite some time, so no pressure to fix it now.

But I would like to merge the fixes you made so far before a release. I'm totally fine with merging this as is. We can see what to do with the open issues / Webpack later.

So is this ready to merge from your point of view?

[nrenner]

eslint-plugin-compat does less than I expected:

• no feature detection support
  doesn't recognize when availability of a feature is checked before using
  • Allow Duck-typing checks for conditional API compat logic · Issue #26
  • e.g. we check if navigator.maxTouchPoints exists before using, it's intended to work in all browsers, no need for polyfilling (but might need checking for navigator itself):
    

    brouter-web/js/Browser.js

    Lines 5 to 6 in 1acd411

    	if ('maxTouchPoints' in navigator) {
    	result = navigator.maxTouchPoints > 0;

• no syntax compatibility checking
  it doesn't tell which browsers don't support specific syntax like const or let, just the parser complains with unspecific errors
  • ES6 features (e.g. generators) not being tested against browsers · Issue #206

[bagage]

It also failed to detect the usage of flat() that is not supported everywhere - so indeed it is not 100% reliable. But maybe better than nothing until migration to webpack?

[bagage]

Anyway it should be good to be merged as-is :).

[nrenner]

Thanks!