refactors var into let/const

[UncleGedd]

Updates Variable Declarations to ES6 syntax

A todo in the codebase was to change var to let (see attachHandlers.js), so I went ahead and updated all of the source files (not including the web extension) to use the appropriate let/const ES6 syntax.

[confusingstraw]

i see you are saying you changed them in "all of the source files", but the first few modified files are the build output. did you change the sources and re-run the builds, or did you change the builds themselves? in either case, it means implicit deprecation of browsers which don't support that syntax (about 3.6%).
here is where i got that number from:
https://caniuse.com/?search=let
if we don't care about IE <11, Firefox <44, etc., then this is fine. i'd imagine we ought to let the build system take care of the legwork regarding let/const though.

[UncleGedd]

The changes in the build files are the result of running the build script before final testing. I guess Rollup decided to do something a little different with the ES6 syntax

EDIT: Oh wait, I see what you mean. Not sure how that happened, let me push up a quick fix.
EDIT2: No fixes necessary, the changes in the build directory were indeed the result of running the build script

[confusingstraw]

ah, seems like you are right. i think it is because we only use babel in our testing, not our building.

i just went down a rabbit hole trying to figure out where the hell that browser detection regex stuff i noticed in the webextension came from. turns out it was added by @poorejc , looks like it is just the inlined content of the library. this isn't relevant in this PR, especially given the change is like two years old at this point, but it is a shame we had to drop being "runtime dependency free" for that browser detection stuff.

to not be entirely unhelpful, i tried to get our build system to do the let/const transformation locally, and was able to get it working with the following changes:

1. add @rollup/plugin-babel and @babel/plugin-transform-block-scoping as dev dependencies
2. update our gulpfile.js to include the following:

// near the top, with the other imports
const {babel: rollupBabel} = require('@rollup/plugin-babel');

//in the gulp.task('rollup'... plugins
        commonjs({ include: /node_modules/ }),
        rollupBabel({ babelHelpers: "runtime", exclude: /node_modules/, plugins: ["@babel/plugin-transform-block-scoping"] }),

doing this seemed to result in a relatively small diff to the build outputs, and transforms any let/const usage to var.

[UncleGedd]

Very nice! I also noticed that we weren't transpiling in our builds and thought it was odd. Glad it was an easy fix!

[UncleGedd]

Regarding detect-browser, @confusingstraw do you know of an elegant way to get around using that dependency? How do you feel about just using the user agent? I know it can be spoofed, but I'm not sure if that is actually a problem for our users. The benefit of being "runtime dependency free" could outweight the potential drawback (at worst, we log the wrong browser?).

[confusingstraw]

personally, im not terribly sure of the value of it at all. @poorejc probably has more context, but it being in the extension build seems pointless, as the return value of detect isnt' even used. in the client library, it seems it is used to extract the browser vendor/version. it does do a little more than use the useragent, but it seems like work that could be done in post-processing. other client fingerprinting techniques that have become popular lately would include webgl/audiocontext fingerprinting, and would probably be significantly more robust.

example of such a library:
fingerprintjs

[poorejc]

@confusingstraw RE 'detect-browser'. Yes, some time ago our core user group made some strong requests for browser characteristics. Did some research and, at the time, 'detect-browser' appeared to check the boxes (well used on npm, upvoted on stack). Fully open to updating to a better library. I'll pull your suggestion into a new ticket, Rob.

NOTE: obviously the request was for inclusion into the client library. No one is using the web extension in production. It's inclusion there is a side effect of our build process.

[poorejc]

personally, im not terribly sure of the value of it at all. @poorejc probably has more context, but it being in the extension build seems pointless, as the return value of detect isnt' even used. in the client library, it seems it is used to extract the browser vendor/version. it does do a little more than use the useragent, but it seems like work that could be done in post-processing. other client fingerprinting techniques that have become popular lately would include webgl/audiocontext fingerprinting, and would probably be significantly more robust.

example of such a library:
fingerprintjs

added to #71

[poorejc]

@UncleGedd does this branch include commits for Gulp refactor? Looking through these comments, it seems like it does. However there are some merge conflicts in the Gulp refactor PR but not this one. Just curious if I should mess around with the previous PR, or go straight to this one.

[UncleGedd]

@JPoore, this PR does not include the commits for the Gulp refactor. Each PR (both this one and the Gulp refactor) started from the master branch. We are using Rollup inside the Gulp build tasks which is why it is mentioned in some of the comments above. Also, I'm happy to pair on the merges if that would be helpful.

EDIT: took care of the merge conflicts in the Gulp refactor PR

[poorejc]

@JPoore, this PR does not include the commits for the Gulp refactor. Each PR (both this one and the Gulp refactor) started from the master branch. We are using Rollup inside the Gulp build tasks which is why it is mentioned in some of the comments above. Also, I'm happy to pair on the merges if that would be helpful.

EDIT: took care of the merge conflicts in the Gulp refactor PR

OK, @UncleGedd awesome. I'll get to testing and pushing these tonight. Sorry, was running a bit behind this week--little busier than expected.