[GitHub] [incubator-flagon-useralejs] confusingstraw edited a comment on pull request #70: refactors var into let/const

2021-03-23 Thread GitBox


confusingstraw edited a comment on pull request #70:
URL: 
https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805222334


   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](https://github.com/fingerprintjs/fingerprintjs)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [incubator-flagon-useralejs] confusingstraw edited a comment on pull request #70: refactors var into let/const

2021-03-23 Thread GitBox


confusingstraw edited a comment on pull request #70:
URL: 
https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-804985422


   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:
   ```js
   // 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`.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org