Skip to content

Comments

fix: disable MSAK client debug mode in production#101

Closed
Kunal1522 wants to merge 1 commit intom-lab:mainfrom
Kunal1522:fix/msak-debug-mode
Closed

fix: disable MSAK client debug mode in production#101
Kunal1522 wants to merge 1 commit intom-lab:mainfrom
Kunal1522:fix/msak-debug-mode

Conversation

@Kunal1522
Copy link
Contributor

Problem

client.debug was set to true in runMSAK(), enabling verbose internal logging to the browser console for every user running a speed test. Debug output should never ship to production.

Fix

-      client.debug = true;
+      client.debug = false;

client.debug was set to true, enabling verbose internal logging to
the browser console for every user running a speed test. Debug output
should not be present in production builds.

Set client.debug to false.
@robertodauria
Copy link
Contributor

Thanks @Kunal1522!

While I agree in general, writing the test state to the console in this case was an intentional choice. Console logs are infrequent enough that they won't interfere with running tests, and I think it's reasonable to expect additional verbosity when looking at the development console.

Perhaps an alternative idea would be renaming debug to verbose in the JS library, to better reflect what it does?

@Kunal1522
Copy link
Contributor Author

hi @robertodauria can i make a separate to rename debug to verbose in case that is not covered in #83 or should i wait for it to be merged ?

@robertodauria
Copy link
Contributor

hi @robertodauria can i make a separate to rename debug to verbose in case that is not covered in #83 or should i wait for it to be merged ?

That change would need to be made to the msak-js library (https://github.com/m-lab/msak-js/blob/main/src/msak.js#L11) and be included in the next release, as the variable is defined there. After looking at what this is actually used for, however, I feel debug isn't a bad name for it. Renaming it would also definitely break existing users of the library. I would just keep it as-is. :)

@Kunal1522
Copy link
Contributor Author

Apologies for the oversight :( I didn’t realize it was defined in msak-js . That makes sense. Thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants