in injectScript don't immediately resolve if matching script is in the dom#194
Conversation
…id is found. Apply an attribute to the script which can be checked to inspect whether the script has loaded or notO
…id is found. Apply an attribute to the script which can be checked to inspect whether the script has loaded or notO
|
Url is variable, for example if we change language or libraries, it will change |
|
Ok, I must've misinterpreted the requirements from the comments in the script primarily these two:
On those grounds the reason for the ID is only to lookup if the script has been written to page yet. If you are loading up new/different libraries then you want to create a new script tag. Removing old script tags provides no benefits other than human aesthetics normally as the code from the script tags is still running on the page regardless of the presence of the script tag. Querying for existing scripts based on the url is the most effective method to ensure that you don't load things up twice isn't it and should provide all the benefits of using an ID and then comparing the url? Current method isn't exactly causing problems though so if it's not broken don't fix it! 😆 |
|
can we close PR and according issue? |
|
Not yet. I have just been double checking things and it solves the issue with injectScript but I think there is an issue with useLoadScript which has logic prior to calling to injectScript that causes the useLoadScript hook to complete early. Will push a fix up in a few minutes. |
…nt for immediately resolving the promise if an element with matching id was found
|
Okay. Not an issue with useLoadScript hook actually. An issue with my own changes - forgot to add an early return statement in where it and also to remove the old document.getElementById check which meant the promise was still resolving immediately if an element with matching id property was found. Have fixed this and pushed it up. |
|
@uriklar @FredyC There is a fairly simple reproduction of the bug here |
|
Well, as I was already mentioning somewhere else, I think it makes total sense to get rid of the Removing old script doesn't matter as it won't be executed again. Besides, Google alone will inject a bunch of other script elements there and we are not removing those anyway. So why to bother at all? I will review the code later, but let's discuss this first. I don't see any benefit of having that |
|
@FredyC removing old html elements does make sense, cos we have performance limitations on total amount of html elements on the page 1500-2000, and this line could be crossed without attention |
|
@JustFly1984 Well, as I said, Google adds a bunch of those linked elements which we are not removing. Removing a single script won't do much of good :) You would have to try to identify which scripts were added there by Google and remove those as well, but that's a slippery slope. You might remove a script that wasn't loaded yet. Anyway, we can basically remove the element right after the Point is that we surely don't need |
|
Thinking about it more, there is a huge limitation in the form of @slorber was trying to achieve that in #188 (comment), but the approach wasn't right, because it would prevent multiple uses of I think that probably the simplest and most robust solution would be adding |
|
@FredyC I don't agree. I want to be able to change script url at runtime, because I need to change the libraries/languages, which requires refetching the script and overriding. It is an SPA, if user changes language, the map should change language at runtime. Your solution could only work for me if you cleanup window.googleUrl on unmount, which imho is not really easy if you allow multiple I think it's safe to prevent the user from using Note that even google logs an error when it gets overriden, there's probably a good reason for it: preventing the user to shoot in the foot. Silently preventing script url changes will just lead to confusion as the user is doing weird things and is not even aware of. Loading google script is hard and annoying on SPAs, particularly when you have to change the conf dynamically. To ensure it's user friendly imho we should fail fast with meaningful error messages which explain clearly the google map constraints, maybe with a link or something. |
|
Wait, what? :) So you have My use case is simply about the separation of concerns. Most pages don't need maps, so it doesn't make sense to be loading them beforehand. When navigating to the page I want to simply use that hook which has 100% stable configuration across the pages because it's wrapped into another hook which defines it. I certainly don't need a different language or whatever within a single app. Developer friendliness resides in being forced to have that hook once on top and load maps always. So unless I have misunderstood your use case, I don't see a point why not to prevent changing the configuration the way I've drafted. There wouldn't be any point in having |
Ok so perhaps we should be thinking in a different less-opinionated direction. Provide the most basic implementation that can load the script, but leave the rest to the developer. No limitations or attempts to gracefully take care of dynamic changes. I don't think there is any gold path here to satisfy the needs of everyone. Btw, in practice, changing the language on the fly is something that happens once at most per the user. We have opted to simply refresh the app in that case because the hassle of dynamically changing everything is totally not worth it to cover that scenario. |
I have an international gatsby site, not all pages use google maps. User can navigate from /fr/pageWithMap to /en/pageWithMap, and the language of google maps must change when this happens. Don't particularly want to refresh the page when this happens, and currently only this lib makes it hard to handle that language change at runtime correctly. If you choose a different path for your app that's fine but do not force everyone to follow your path. I don't necessarily want to load google maps for all pages, I don't reco to put a global hook for all the site's pages but rather, at the top of each page's body, just under the main site layout, on a per-usecase basis, choose to use the hook or not. It's the same for libraries. I could load all libraries, but if page1 use lib1, and page2 use lib2, do I really want to load lib1+lib2 if user never visits page2? (actually in this case I do load all the libs because I think the overhead might not be significant)
I've successfully made hacks in my app but those were not easy. On the contrary I think this lib should provide things to solve the pains of this issues. I don't understand why you really need, in your app, to call the hook multiple times, what's the usecase for this? What would be complicated to migrate your multiple calls to the approach I suggest above, which is to put a unique hook somewhere upper in your tree, instead of having duplicate deeper hooks. As google maps is a singleton, I really feel it's a bad idea to let user call twice the hook on the same app. It's just making things less explicit. As a google maps newbie 1 month ago, I wasn't even aware that google maps was loaded this way and all the subtilities. Had to see only after production that the google maps language did not change correctly on page language change. For me the hook should not fail safe in an unexpected way that lead to production i18n bugs, but rather encourage the developer to do the right thing, not hide him the fact that google maps is a singleton, and educate him on how to handle this annoying singleton properly |
|
@FredyC reloading an app is not an option, if you have a delivery form UX, half filled in, for example, and that is just first thing comes to my mind. The main goal I have made this lib rewrite from beginning is to be able to switch language in runtime without page reload. and to get rid of 150k of lodash, but that is an offtopic. |
I would really love to see the user who starts filling the form which is in the wrong language and then he decides to switch language just because it sounds like a good idea 😆 Doesn't matter if you consider it as a necessary evil, it's understandable. I kinda had this approach before as well, but I see it as micro optimization now.
It's not about calling hook multiple times, I don't do that. I do have a BaseMap component which calls it. So whenever I use that component, I want it to load scripts and be happy. What bothers me is that I would have to include it into some top-level Page component as something totally unrelated and control it through the app state if maps should be loaded or not. How is that developer-friendly compared to just using the hook? |
|
I have an order form where user can switch country but keep page language. Switching country should trigger map language change because we want Google map addresses to be localized to the order country. This is mostly because in our business we want to support an user with French locale to order for us country and vice versa. I understand that you might want to call the hook collocated to the comp that requires Google maps. But this hides the fact there is a singleton. As a newcomer using this lib it can make you think you can display 2 maps in 2 distinct languages at the same time. As I said you should absolutely not put the hook at the very top. But you'd rather put it on a per page basis. I'll put this in OrderPage on my usecase because I know Google maps is required on the order process. But if it's too high for you, just find the closest parent of all your comps requiring Google maps. Note that if you only use one map comp per page, you'd still be able to colocate. We may even nooo if you use 2 hooks and they share the same url as it's same. But if you start using 2 hooks with different config, I really think we need fail fast, because weird things will start to happen. |
|
Alright, I gave it some thought and it should probably work. I would even say it clicked in my head with your last comment about the singleton behavior of Let's get back to the questions of this PR then. Should we keep the |
|
Reading other comments in this thread it sounds like the desired effect, in order to mimic window.google's singleton nature is to only allow one google script tag on page at a time. Therefore it sounds like the script element should remain on page until/if a different google maps configuration is loaded up? Documentation should reflect that changing configuration will cause a refresh of the window.google object to highlight the point raised by @slorber about trying to display two maps in different languages on page at the same time. Also the development build should probably console.warn on configuration change to notify the developer about this as well. Does the above sound correct to everyone or have I missed anything/made some wrong assumptions? If there is agreement I will update the PR later today. |
|
I think you got it right. Personally, I wouldn't really bother with removing old script element, it's really an micro-optimization, but as long as it's not removed too soon, it should be fine. Not sure why you want to keep |
|
@coulson84 you are correct, there is no possibility to load more than one google maps script, due to Singleton nature of ‘window.google’ object. But there is a clear requirement to change language or libraries, or region e.g url at runtime. |
|
Url can change, I’d can’t change |
|
@FredyC removing the old script is for humans more than anything else. So it is more apparent to a dev nosing around the DOM which google maps script/config is active (if they have an app using > 1 config). If only using one script tag at a time then we may as well use an id as we are already. It means the fewest changes to the code is probably marginally quicker than querying by tag+src and also means that if some one is loading their own google maps script, outside of this library for some reason. We will not accidentally target that script. I'll go ahead and update the PR. |
Well, if someone would be that crazy to have Perhaps the I am thinking it's actually a fragile approach to check for the existence of element to decide if we want to inject one or not. What if we really use
Not sure I got that. Quicker you mean to code that or performance-wise? That would require measurement, but I have doubts it's marginally quicker if that's what you meant :) |
|
@FredyC I don't see how using a global variable is any less fragile than checking for the presence of a script element. They are both globally available and open to being manipulated something local to the module code should do the job though Thinking about this more. The removal of the id property is basically a breaking change and would require a major version update. There's no way of knowing what someone has already implemented on the assumption that the id they pass in is applied to the script and removing that behaviour will break whatever they have implemented so this behaviour needs to remain the same if you want to avoid a major version increase. It might be possible to change the behaviour without a major version hike based on the fact that the changes are to undocumented behaviour that the user should not be relying on anyway... I guess 😕 . Do you see where I am coming from? |
|
It's unlikely anyone would care about Either way, I wouldn't be afraid to do a major version. We could even drop the old LoadScript and replace it with LoadScriptNext that utilizes the hook. But in that case, we should have V2 branch and merge a couple of PRs there before merging it to master.
It just feels like DOM is something slightly more fragile, not even mentioning it's slower to interact with it than with global variables. Also, we could use the private |
| script.async = true | ||
| script.onerror = reject | ||
| script.setAttribute('state', 'loading') | ||
| script.setAttribute('data-state', 'loading') |
There was a problem hiding this comment.
Do we actually need to set that attribute at this point? Since the only check for ready is done and getAttribute will return null, there is no need to tweak DOM more than necessary imo.
|
Okay. If doing a new major version isn't an issue then I'd like to merge this PR as it stands. I'll do the other work discussed on this thread as a second pull request. The reason for splitting these out is that I want the bug fix merging and releasing as soon as possible as I want to use the bug fixed version in our app for work so we push it out in the next release cycle. If all of this work starts to roll in to a new major release then it will probably be delayed by quite a bit. |
danielkcz
left a comment
There was a problem hiding this comment.
I think it makes sense to keep changes here minimal to get it out and we can do 2.0 later.
| initMap?: () => void; | ||
| } | ||
|
|
||
| // interface ScriptWithOnErrorHandler extends Element { |
There was a problem hiding this comment.
Can you drop these commented out parts unless you want to revive those later?
| }) | ||
|
|
||
|
|
||
| // it('returns a promise which rejects if the script does not load', () => { |
There was a problem hiding this comment.
I wonder why this test doesn't work. Is it some technical limitation?
There was a problem hiding this comment.
I think it is a limitation with jsdom. It allows setting an onerror handler for the script but always returned a null value when you tried to retrieve it.
| return resolve(id) | ||
| } | ||
| else { | ||
| const onGoogleMapsReadyClosure = windowWithGoogleMap.initMap |
There was a problem hiding this comment.
It's kinda hard to understand what's going on here. Had to read it several times to understand it's actually calling original version of that function to resolve all promises in the row. Perhaps even some comment would be useful.
| const onGoogleMapsReadyClosure = windowWithGoogleMap.initMap | |
| const originalInitMap = windowWithGoogleMap.initMap |
danielkcz
left a comment
There was a problem hiding this comment.
@JustFly1984 I think it's safe to merge this, the underlying change is fairly small and shouldn't break anything.
|
@JustFly1984 I don't think the minified script for v1.5.2 has been updated to match the latest unminified content. I can't find any occurrence of |
|
@coulson84 FYI, the latest release should be alright now. |
When injectScript runs it checks for the presence of a script element with matching ID in the DOM. If it finds it, it immediately resolves, regardless of whether the script has finished loading or not.
This changes add a 'state' attribute to the script that can be used to check if the google maps is ready or not and then use that information to decide whether to resolve immediately.
There are a couple of possible issues. I would probably prefer using
script.addEventListener('load', callback)instead ofscript.onloadand then using the closure to maintain a reference back to the original call to injectScript's onload callback. That's just what I prefer but attempting with changing the source as little as possible to implement the feature.I have seen in other threads the mention of removing the id from the call but that it was needed to to find the script via document.getElementById. Why not use the below to find scripts according to just the url? I am not sure if this has been considered but found to be not suitable or not?