Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polyfill does not always fire on iOS #363

Closed
bakura10 opened this issue Mar 17, 2023 · 19 comments · Fixed by #364
Closed

Polyfill does not always fire on iOS #363

bakura10 opened this issue Mar 17, 2023 · 19 comments · Fixed by #364
Labels
bug Something isn't working

Comments

@bakura10
Copy link

bakura10 commented Mar 17, 2023

Hi,

I have tried to migrate our code to using importmap and, under iOS (I tried on 16 and 15) it sometimes work, and sometimes does not (navigating between pages cause the issue):

image

While sometimes, it will work:

image

I am not doing anything fancy here (asset_url is a Shopify construct to generate the URL of a given resource, but this happens server side so on the client the complete URL is just rendered):

<script async src="{{ 'es-module-shims.min.js' | asset_url }}"></script>

    <script type="importmap">
      {%- comment -%}On Safari 16.3 and lower, a polyfill is used to load importmap{%- endcomment -%}
      {
        "imports": {
          "vendor": "{{ 'vendor.min.js' | asset_url }}",
          "theme": "{{ 'theme.js' | asset_url }}",
          "photoswipe": "{{ 'photoswipe.min.js' | asset_url }}"
        }
      }
    </script>

    <script type="module" src="{{ 'vendor.min.js' | asset_url }}"></script>
    <script type="module" src="{{ 'theme.js' | asset_url }}"></script>

What could be happening here that would cause the polyfill to sometime render, sometime not?

Thanks.

EDIT : from what I can see, due to the fact the script is async, it seems to fire up before the other modules, so the TypeError is fired after the polyfill being executed:

image

@guybedford
Copy link
Owner

Can you share a replication of the issue that I can test out here? Yes there are a number of different timings that can apply for the polyfill depending on the exact network order and how the page is set up. All of the various cases should be properly handled through.

@bakura10
Copy link
Author

Hi @guybedford ,

Thanks for your answer :). I don't have a small reproduction case, but you can replicate it here: https://prestige-new-version.myshopify.com/ (to enter you need to enter the password rewblo)

Then, on iOS Safari, navigate between different page and, on the home page, you will find that sometimes, nothing will load. This also happens on Safari on Mac but, somehow, it seems to reproduce a bit less.

It may be due to some scripts that Shopify may load but I really could not identify the root cause.

@guybedford
Copy link
Owner

@bakura10 I was able to verify your case here, but we definitely some better debugging information. I've put together some simple logs in #364.

Here's a copy of the build if you can update your app to that - https://gist.github.com/guybedford/e56e8357bd2cf5fc2b199fd4fba38f00. Will be great to try and dig into it further from there if you're open to iterating on this, given we don't have an isolated replication.

@bakura10
Copy link
Author

Hi @guybedford ,

Sorry for not being able to setup a small reproduction case, I added this library at the very end of our development. The debug build is now on the store.

When the error occurs, I unfortunatley don't have a lot of extra information, just this:

image

@guybedford
Copy link
Owner

@bakura10 the problem here is actually clear in the message thankfully! baseline passthrough is not supposed to be displaying as that means it thinks the browser already supports import maps.

I've added some more logging into the feature detection system to try and dig down into this. I think it's thinking import maps are supported when they aren't, so some of those logs might help figure out why.

Updated debug build at https://gist.github.com/guybedford/e56e8357bd2cf5fc2b199fd4fba38f00.

@bakura10
Copy link
Author

Hi @guybedford ,

I have added this new debug build. Here is now the new error:

image

I'm not able to explain this error, the variable seems to be defined :/

@guybedford
Copy link
Owner

@bakura10 apologies, I've updated the logging in https://gist.github.com/guybedford/e56e8357bd2cf5fc2b199fd4fba38f00 again. Hopefully we should get something useful now.

@bakura10
Copy link
Author

bakura10 commented Mar 21, 2023

No problem @guybedford . I have updated the script. Here is what I get now:

image

It seems that, at least on iOS 16, import maps are marked as supported while they are not.

Hope this helps :)

@guybedford
Copy link
Owner

Thanks @bakura10, yes this narrows it down to the import map feature detection not working in Safari. I've included a possible fix in the new debug build if you want to try it - https://gist.github.com/guybedford/e56e8357bd2cf5fc2b199fd4fba38f00.

@bakura10
Copy link
Author

Hi @guybedford . I have tried this new build and it seems to fix it. I am going to do some more tests on all the pages to ensure this is the case.

Out of curiosity, what was the issue? I am a bit surprised that what is likely one of the main purpose of this library (supporting Safari for importmaps) failed at something as basic as detecting the support. Is it something in our code or the platform we are using (Shopify) that caused this issue?

@guybedford
Copy link
Owner

@bakura10 so this is a bit of a tricky one because the feature detection script is passing indicating that import maps are supported, when they aren't. And the feature detection script itself seems to be working perfectly fine in isolation as far as I can tell. See https://guybedford.com/feature-detect.html and look at the browser console for an isolated version of this.

So something about this Shopify setup is breaking the feature detection script, and I can't work out how that might be happening.

I can't actually ship that debug fix as-is as that the fix I posted entirely disables feature detection while we do very much need feature detection for Chrome versions.

I can try adding some more logs to the feature detection system itself to see if we can dive deeper into the issue - without a full replication that's about as good as we can do right now.

Would you be open to further debugging on this?

@bakura10
Copy link
Author

Of course :). We plan to release this project in 3 weeks, but this is currently a bit of a breaker so if I can't find a solution I will need to give up on importmap. I will do some debugging on my end as well to see which part may be causing the detection to fail.

@guybedford
Copy link
Owner

@bakura10 I'm happy to keep pushing on this as it would be great to narrow down and it feels like we're making good progress already. It may only take a couple more iterations to work out the correct fix.

Latest debug build at https://gist.github.com/guybedford/e56e8357bd2cf5fc2b199fd4fba38f00 if you can share the logs.

@bakura10
Copy link
Author

Hi @guybedford :)

Here is what I get with this new build (it is not working on this one):

image

@guybedford
Copy link
Owner

@bakura10 thanks for the update, I was able to track down the issues with the feature detection script! This is a really huge bug to catch so thank you.

I believe I should have a proper fix working in the debug script at https://gist.github.com/guybedford/e56e8357bd2cf5fc2b199fd4fba38f00. Please try that out and let me know how the logs look to confirm that it is working correctly.

@guybedford guybedford added the bug Something isn't working label Mar 26, 2023
@bakura10
Copy link
Author

Hi @guybedford . Thanks on helping for this one, this is highly appreciated. I have updated the build. It seems to work consistently on the few pages I have tried. Here is the complete log:

image

Hope this helps :)

@guybedford
Copy link
Owner

Good timing here, as I just updated my phone to the latest iOS which has native support. The fixes are released in 1.7.1. Thanks again for the help.

@bakura10
Copy link
Author

Awesome. Thanks a lot. This is minor detail, but just to let you know that the link in the doc does not work: https://ga.jspm.io/npm:es-module-shims@1.7.1/dist/es-module-shims.js

@guybedford
Copy link
Owner

@bakura10 thanks for the ping on that, sorted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants