You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flagon.apache.org by GitBox <gi...@apache.org> on 2021/03/23 12:01:19 UTC

[GitHub] [incubator-flagon-useralejs] UncleGedd opened a new pull request #70: refactors var into let/const

UncleGedd opened a new pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70


   # Updates Variable Declarations to ES6 syntax
   
   A todo in the codebase was to change `var` to `let` (see [attachHandlers.js](https://github.com/apache/incubator-flagon-useralejs/blob/cf8aaaf2eb36121a52aa87755d1ddccbfe5f6509/src/attachHandlers.js#L23)), so I went ahead and updated all of the source files (not including the web extension) to use the appropriate `let`/`const` ES6 syntax.


-- 
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
confusingstraw commented 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:
   [https://github.com/fingerprintjs/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:
users@infra.apache.org

[GitHub] [incubator-flagon-useralejs] UncleGedd commented on pull request #70: refactors var into let/const

Posted by GitBox <gi...@apache.org>.
UncleGedd commented on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-804943594


   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


-- 
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:
users@infra.apache.org

[GitHub] [incubator-flagon-useralejs] UncleGedd commented on pull request #70: refactors var into let/const

Posted by GitBox <gi...@apache.org>.
UncleGedd commented on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805056097


   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.


-- 
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
UncleGedd edited a comment on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805715431


   @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


-- 
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:
users@infra.apache.org

[GitHub] [incubator-flagon-useralejs] poorejc commented on pull request #70: refactors var into let/const

Posted by GitBox <gi...@apache.org>.
poorejc commented on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805393984


   @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. 


-- 
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
UncleGedd edited a comment on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805056097


   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?).


-- 
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [incubator-flagon-useralejs] asfgit merged pull request #70: refactors var into let/const

Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70


   


-- 
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
confusingstraw commented 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, as 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:
users@infra.apache.org

[GitHub] [incubator-flagon-useralejs] UncleGedd commented on pull request #70: refactors var into let/const

Posted by GitBox <gi...@apache.org>.
UncleGedd commented on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805715431


   @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.


-- 
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
UncleGedd edited a comment on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-804943594


   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.


-- 
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:
users@infra.apache.org

[GitHub] [incubator-flagon-useralejs] UncleGedd commented on pull request #70: refactors var into let/const

Posted by GitBox <gi...@apache.org>.
UncleGedd commented on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805043818


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


-- 
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:
users@infra.apache.org

[GitHub] [incubator-flagon-useralejs] poorejc commented on pull request #70: refactors var into let/const

Posted by GitBox <gi...@apache.org>.
poorejc commented on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805436438


   > 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)
   
   added to https://github.com/apache/incubator-flagon-useralejs/issues/71


-- 
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:
users@infra.apache.org

[GitHub] [incubator-flagon-useralejs] poorejc commented on pull request #70: refactors var into let/const

Posted by GitBox <gi...@apache.org>.
poorejc commented on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-808615924


   > @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.


-- 
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
UncleGedd edited a comment on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-804943594


   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 were indeed the result of running the build script


-- 
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:
users@infra.apache.org

[GitHub] [incubator-flagon-useralejs] poorejc commented on pull request #70: refactors var into let/const

Posted by GitBox <gi...@apache.org>.
poorejc commented on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805451165


   @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.


-- 
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
UncleGedd edited a comment on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-804943594


   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


-- 
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
UncleGedd edited a comment on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805056097


   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 a different browser?).


-- 
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
UncleGedd edited a comment on pull request #70:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/70#issuecomment-805043818


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


-- 
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:
users@infra.apache.org