Hi guys!
Hi guys!
You have a car, you are driving along the road.
Suddenly your car's radio breaks down, stopping your car completely and blocking the doors and engine.
Do you think this is the correct behavior?
27 Replies
Unfortunately, your library completely breaks my machine. 😩
context please, so that we can fix the radio and get you back on the road
https://github.com/thoughtspot/visual-embed-sdk/issues/920
One part of my car is completely blocking my entire car.
GitHub
Alert messages in the my application are blocking my interface · Is...
Please remove any alert() from your code because its are blocking my application interface. Sometimes I can see alert message alert(DUPLICATE_TOKEN_ERR); I don't know why this happens, but it b...
Our clients use our product in emergency systems, and if during a critical call, the interface suddenly becomes blocked due to a problem in a secondary analytics module, then this will be a very big problem.
Yes, we are actively working on this. We should be able to publish the sdk with fix by tommorrow. If you need it sooner we can release a patch with duplicate token check commented out.
PR: https://github.com/thoughtspot/visual-embed-sdk/pull/1023
The duplicate token problem is a "radio" problem, but it shouldn't break the whole machine!
Third-party libraries should not use alerts because it can completely break the entire system.
Agreed, as per the documentation a new token needs to be fetched for each call to
getAuthToken https://developers.thoughtspot.com/docs/Interface_EmbedConfig#_getauthtoken ...
The alert is shown when the developer does not follow that. So that the dev sees this while development and the user is spared from unexpected auth errors.
Now, I agree for cookieless tokens with a longer expiry this might not be necessary, hence we are making the above change.// eslint-disable-next-line no-alert
alert(DUPLICATE_TOKEN_ERR);
I see alert in the new place 😦
Maybe it’s not for nothing that eslint complains about this line?
Yes, if the token is invalid we still want to make sure this is caught early in the dev/test cycle.
Well, these are our own eslint rules 🙂
One does not get any errors if the docs are followed. But as I said, we are relaxing the constraint because it might be too strict for new authentication types we have introduced.
Looks like you didn't understand our problem. We generally do not want any secondary module to somehow break our entire system.
Give us the option to completely disable alerts when configuring the client
{silentErrors: true}
Yes, we can do that. But then, there might be cases where due to wrong implementation on your side the "radio" wouldn't work. And it will be seen by the user and not be caught in testing/dev. Is that okay ?
I would rather have people fix the error, than do
silentErrors: trueIf the analytics module breaks, then our client can survive it, but if the analytics module breaks and then breaks the entire system, then this is already a problem.
The analytics module is built into our system and it is better for us to have one broken module than the entire broken system.
Alright, we will add that option.
Our rationale was that, this
alert is so in your face that the code will never reach the customer and would be caught and fixed in some layer of CI/CD.If this is used in a separate system, then this can be done, but in our case it may someday break our entire system and we want to be able to disable it
Thanks for understanding!
Sure, sorry for the inconvenience.
One follow up question, do you guys have
env flags or something? Like process.env.NODE_ENV = 'production ? We could decide the default value of the silenceErrors flag based on thisThis happens in the browser, there are no such process.env variables
Yes, but sometimes there is a build process which replaces the flag based on the target environment.
rollup/webpack
init({
thoughtSpotHost: '<%=tshost%>',
authType: AuthType.None,
silentErrors: true
});
Is it possible solution?
yeah, thats what we will do. I was curious to see if you guys had something for it to be automatically inferred. But, anyways we will expose it.
I really hope that the new setting will be able to disable all types of alerts?
I suggest replacing all alert calls in your SDK with similar code
function customAlert(message) {
if (options.silentErrors !== true) {
alert(message);
} else {
console.error(
The error was hidden, but not resolved, message);
}
}This one can already be suppressed https://developers.thoughtspot.com/docs/Interface_EmbedConfig#_suppressnocookieaccessalert
EmbedConfig
The configuration object for embedding ThoughtSpot content. It includes the ThoughtSpot hostname or IP address, the type of authentication, and the authentication endpoint if a trusted authentication server is used.
But yeah the new flag should disable all
Hi guys! Alert for Third-party cookie disabling it is a god idea, but my main problem with alert(DUPLICATE_TOKEN_ERR); and it isn't solved 😦
You have many alerts in your code. It is a very bad idea to use it. Each one can block the user's interface!
@Volodymyr Horobets we have published 1.26.2 , where we have introduced
suppressErrorAlerts to disable all the alertsWhat has happened with 1.26.2? https://github.com/thoughtspot/visual-embed-sdk/pull/1033
@Volodymyr Horobets 1.26.2 has already been merged long back and avialbable here : https://www.npmjs.com/package/@thoughtspot/visual-embed-sdk
npm
@thoughtspot/visual-embed-sdk
ThoughtSpot Embed SDK. Latest version: 1.26.2, last published: 2 months ago. Start using @thoughtspot/visual-embed-sdk in your project by running
npm i @thoughtspot/visual-embed-sdk. There are 4 other projects in the npm registry using @thoughtspot/visual-embed-sdk.