Improve Acuant Global Code Comment#10247
Conversation
night-jellyfish
left a comment
night-jellyfish
left a comment
There was a problem hiding this comment.
Thank you for updating this!
| * Global declarations | ||
| */ | ||
| declare let AcuantJavascriptWebSdk: AcuantJavascriptWebSdkInterface; // As of 11.7.0, this is now a global object that is not on the window object. | ||
| // As of 11.7.0, Acuant provides global objects that are not on the window object. We (identity-idp) |
There was a problem hiding this comment.
I'm a bit confused by this, since I equate "window" and "global" in the context of a browser. How are they different? Or is this something in a worker?
In a web browser, any code [...] has a Window as its global object.
https://developer.mozilla.org/en-US/docs/Glossary/Global_object
There was a problem hiding this comment.
This is a good point. I do know that Acuant uses workers, but frankly didn't think too hard about this. I was/am leaning on the writeup here pretty heavily. Specifically:
In 11.7.0, this is no longer the case -- these objects are injected into the browser's global scope, but not added to the window object. This introduced three breaking difficulties: where to "find" the correct resources; the assumptions of our frontend test suite; and the assumptions of our type annotations.
I just added ( commit ) a link to that in the comment, since that's really the information a future reader needs.
There was a problem hiding this comment.
Is it at all possibly linked to the shadow DOM? Maybe the shadow DOM's global is not the window?
It sounds like from the documentation that window === global. But the history and comments in our code says otherwise. I don't feel I understand this level of how it all works (though I'd like to learn).
There was a problem hiding this comment.
It's quite possible we were just mistaken in some of that older code?
What happens if this line of code is removed altogether?
There was a problem hiding this comment.
I removed it and a couple other lines complained about it missing. I updated them the best I could guess and it...seems fine? I do think there are still some problematic comments / code like here and here and probably others. Made a ticket for that: LG-12726.
But I think fixing them would be a bigger lift than this work was originally meant to be, and should probably be ticketed and prioritized, as changing it offhand could break the SDK.
charleyf
changed the title
|
Charley and I agreed I'd take this work on - but I can't seem to remove my approval. I think since I'm taking it on and I made a fairly big modification, it would be good if someone other than me could test / review. |
| // eslint-disable-next-line no-console | ||
| console.error('AcuantJavascriptWebSdk is not defined in the global scope'); |
There was a problem hiding this comment.
Not in scope of what you'd aimed to tackle, but I wonder why we log a message here. If we wanted to log something for us to be aware of, I'd think we'd use trackError. Or if it's for tests (to trip the browser log detection), I'd think we could wrap that in process.env.NODE_ENV. If it's for local development, I wouldn't think we should have it part of the committed code.
There was a problem hiding this comment.
That's a good point. I'm not sure what the original intent of this was either, tbh. It was added almost 2 years ago and I don't see a direct reference to the "why" for the console error.
I think reading through it, my take on it would be that we'd want to know if it's happening in production. The user would probably be encountering some kind of error already if there is no window.AcuantJavascriptWebSdk. And I think we'd want to know if that's happening to users (especially if it's happening to many).
So I think TrackError makes sense. (TIL trackError exists! I thought we always used trackEvent). I created LG-12733 to track that work. I found we use it twice in this file as well.
3 participants
🎫 Ticket
This work happened because of this Slack thread.
🛠 Summary of changes
This PR updates a comment to be clearer.