Shopify / ui-extensions

MIT License
268 stars 36 forks source link

POS UI extensions seriously broken in the latest iOS Shopify POS version #2299

Closed galmis closed 2 months ago

galmis commented 2 months ago

Please list the package(s) involved in the issue, and include the version you are using

Initially found using @shopify/retail-ui-extensions-react version 1.1.2, but reproducible using the @shopify/ui-extensions and @shopify/ui-extensions-react latest versions (2024.4 and 2024.7).

Describe the bug

Steps to reproduce the behavior:

I've received complaints from my app subscribers that my customer barcode scanning app is no longer working since the latest Shopify POS update (9.16.0 I believe).

An easy way to debug it is to

import {  useScannerDataSubscription, Text } from "@shopify/ui-extensions-react/point-of-sale";

//... Use within a screen component, some code omitted for brevity

const scanResult = useScannerDataSubscription();

return <Text>scan: {JSON.stringify(scanResult)}</Text>
  1. Close and reopen the Shopify POS app
  2. Open a POS UI extension - "Something went wrong. Please check your cart or try again." error shows up at the very top. This error doesn't come from the UI extension, but from Shopify POS!
  3. Scan - seems to work ok
  4. Close and reopen the extension and scan again - this may work sometimes, but usually the error no longer shows up and the new scan result doesn't show up after scanning either.

Also note, that when I was debugging this on my dummy unreleased UI extension, the scan result (and some other local state) was persisting when reopening the extension. It'd show scan: {} the first time and update after scanning, e.g. scan: { "data": "some-scanned-text", "source": "external" }, but then would always show the same result irrespective of what was scanned. After scanning and closing the extension the first time, "2 products couldn't be found and were removed" error shows up in Shopify POS under the cart. Opening and closing the extension increments the number of products by 2 (e.g. to 4, 6, 8, etc.).

Also, local UI extension updates have become really slow, making it difficult to test ideas.

Expected behavior

It's likely to be the same root issue (stale extension screens) as raised in https://github.com/Shopify/ui-extensions/issues/2286. Could this please be looked at urgently? POS UI extensions are unusable.

xqwtsz commented 2 months ago

I can also report that useScannerDataSubscription is broken since the 9.16.0 of the iOS Shopify POS App

The scan data only gets updated initially once, if the custom POS UI Extension tile is closed and reopened again, the scan data is null and does not get updated, regardless of the source.

xqwtsz commented 2 months ago

After also checking #2286 i can report that our other POS UI Extension is experiencing similar issues with cart items getting updated.

Please fix this urgently as all extensions are messed up right now.

js-goupil commented 2 months ago

@xqwtsz @galmis Thanks for reporting. Are you experiencing this with the 3P physical scanners or the camera scanner?

xqwtsz commented 2 months ago

@js-goupil both

kritop commented 2 months ago

I can only double down on the previous posts. This is business critical since it affects ALL pos ui extensions since POS release 9.16.0.

js-goupil commented 2 months ago

@kritop @xqwtsz Understood. We're working on a fix, it looks like the issue is related to Subscribables, specifically with the way the useCartSubscription (for example) hooks work, as they don't seem to be removing the previous subscribers when the modal closes. From what I understand, manually subscribing to the subscribers on the cart and scanner objects, and then removing the subscription (subscribing returns a callback to unsubscribe) when your modal closes should resolve the issue for now, as we look to implement a fix on the POS side so that this doesn't happen again.

xqwtsz commented 2 months ago

@js-goupil even if i did switch to the manual subscriptions, and how do you recommend tracking the modal open/closed state? there's only: api.smartGrid.presentModal() that opens the modal, but no way to hook into the native "X" close buttons. I also realised that the component does not really unmount, so I cannot use any unmount lifecycle triggers. apparently onNavigateBack works, thanks @galmis

the sources you linked are incorrect, as they do NOT return the subscribable itself but the end result object, therefore do not have access to the destroy() function. the correct sources are here for the scanner and here for the cart

in any case, dont you have a snapshot that you can rollback to for the iOS application and release it?

we've already informed you from different channels that this is effecting business in the retail locations, its already been a week since the release that introduced the bug and no fix in sight

galmis commented 2 months ago

@js-goupil thanks for the suggestion. I don't know if it's of any help to you guys, but I've noticed that global variables (i.e. outside of the component scope) are retained when reopening app extensions (great if you want to cache something, but breaks a lot of existing functionality). So, new subscriptions aren't created after reopening an extension, because statefulScannerDataSubscribable already contains a (invalid) subscription.

/**
 * A hook utilizing the `makeStatefulSubscribable` function to allow multiple scanner subscriptions.
 * @returns StatefulRemoteSubscribable object with a scan result in it.
 */
export function useStatefulSubscribableScannerData(): StatefulRemoteSubscribable<ScannerSubscriptionResult> {
  const api = useApi();
  if (!isScannerApi(api)) {
    throw new Error('No scanner api found');
  }

  const {scannerDataSubscribable} = api.scanner;

  if (!statefulScannerDataSubscribable) {
    statefulScannerDataSubscribable = makeStatefulSubscribable(
      scannerDataSubscribable,
    );
  }

  return statefulScannerDataSubscribable;
}

@xqwtsz I think you could do something similar to

let unsubscribeScanner: (() => void) | null = null;

export function useScannerDataSubscription() {
  const api = useApi("pos.home.modal.render");
  const [result, setResult] = useState<ScannerSubscriptionResult>(
    api.scanner.scannerDataSubscribable.initial
  );

  useEffect(() => {
    async function subscribe() {
      try {
        const subscription =
          await api.scanner.scannerDataSubscribable.subscribe((result) => {
            setResult(result);
          });

        unsubscribeScanner = subscription[0];
      } catch (e) {
        console.error("error subscribing to scanner data", e);
      }
    }

    subscribe();

    // NOTE: the cleanup isn't fired, but it should according to idiomatic React
    return () => {
      if (unsubscribeScanner) {
        unsubscribeScanner();
      }
    };
  }, []);

  return result;
}

export async function destroyScanner() {
  if (unsubscribeScanner) {
    await unsubscribeScanner();
  }
}

And then call destroyScanner from your main Screen onNavigateBack callback. This seems to work, but I'm not 100% sure the unsubscribeScanner is what I suspect it is. It would make sense to return a destroy function there.

I keep finding other problems, e.g.

etc. So, I'm still trying to identify and fix all of those. I agree with @xqwtsz, this should be reverted. I've noticed issues on Android, as well and I'm worried hacky workarounds could cause more harm than good, especially after another Shopify POS update. It's very disappointing we're in this situation now.

xqwtsz commented 2 months ago

@xqwtsz I think you could do something similar to

this only works only one scan also, i tested that the onNavigateBack is def triggering as i can do other changes inside the callback of this function, but i get no fresh data in the consecutive scans.

but again, i completely agree with:

I'm worried hacky workarounds could cause more harm than good

especially when these files have not been touched in the last 9 months, idk why they would be the culprit now?

i also get flashes of

Something went wrong. Please check your cart or try again

even if i switch to the manual subscriptions

js-goupil commented 2 months ago

@xqwtsz I understand your frustration, we're working on it. This is an issue that's deeply rooted in the lower levels of ui extensions, not necessarily POS. We're well aware of the impacts that this has on merchants. Thank you for your patience.

js-goupil commented 2 months ago

I agree with @xqwtsz, this should be reverted. I've noticed issues on Android, as well and I'm worried hacky workarounds could cause more harm than good, especially after another Shopify POS update. It's very disappointing we're in this situation now.

@galmis I appreciate the constructive feedback, we're working on rolling back the change that caused the caching issues via a hotfix. We'll let you know when we've got a build submitted to the app store and google play store.

js-goupil commented 2 months ago

@xqwtsz @galmis Just a heads up that rollout for iOS and Android for 9.16.0 was paused at 20%, which should stop the bleeding for now.

js-goupil commented 2 months ago

So, new subscriptions aren't created after reopening an extension, because statefulScannerDataSubscribable already contains a (invalid) subscription

@galmis yes that appears to be correct. I'll explain more with what's happening on the POS side, but for now you can remove this logical check in your node modules and you should be good. Here is the same thing for the scanner subscription

So I can confirm a couple things with you guys for transparency, the breaking change is that we shipped caching for UI Extensions. The benefit is much faster rendering. Obviously, subscribables are bugged now. This line of code seems to be causing problems on my end, and removing it fixes the issue. Here is what's happening:

  1. Extension opens, and subscribes the first time. That !statefulSubscribable is true because no one has subscribed before. So then we create a statefulSubscribable based on the current api object, and it works properly.
  2. Extension closes, and POS kills the subscribable on its side (the entire api is destroyed). On the extension side though, that statefulSubscribable still exists in the global (the object is still set there).
  3. Extension re-opens, and the statefulSubscribable is still there so the !statefulSubscribable returns false, and we return the old statefulSubscribable, but really it's pointing at nothing, since the previous api object that it wrapped was destroyed.

Can you edit that code in your esm directory of your build for you node_modules and let me know if that fixes it for now? We are still planning on rolling back caching, but at least you can get a quick fix to your merchants until that hits the app store.

cc @xqwtsz

js-goupil commented 2 months ago

@derrickrc can you test the above as well?

derrickrc commented 2 months ago

@js-goupil thanks for your quick response, we made this change and redeployed our tile, and our prod version appears to be working as expected again.

For anyone else reading, this is where we changed it: extensions/extensions/<YOUR APP NAME>/node_modules/@shopify/retail-ui-extensions-react/build/esm/extension-api/cart-api/cart-api.mjs

js-goupil commented 2 months ago

Update: We have found a fix for the root cause of the issue. We're hoping to get it released in the next couple days (takes time to get through the app store and google), but hopefully tomorrow. @derrickrc, and anyone else who implemented the temporary hack, when this fix hits the app store we strongly recommend you revert (and test of course) to avoid any weird side effects that might happen.

adamwooding commented 2 months ago

Hi @js-goupil , is there more information about this new caching feature / functionality? How can POS UI Extensions take advantage of this?

galmis commented 2 months ago

Thanks for the details @js-goupil and a potentially quick fix, really looking forward to it!

I tried commenting out the if (!statefulScannerDataSubscribable) { statement, and that was my initial idea, but I can see the subscribable being very quickly and infinitely recreated pretty much halting the whole Shopify POS app. This was tested against the new 2024.4 and 2024.7 @shopify/ui-extensions-react packages. Presumably, it's not an issue on the older versions (prior to the migration to the shared extension packages) as mentioned in https://github.com/Shopify/ui-extensions/issues/2299#issuecomment-2313261204.

Reproduction snippet:

let count = 0;

export function MyScreen() {
  const scanResult = useScannerDataSubscription();

  count += 1;

  return (
    <Screen name="MY_NAME" title="My title">
      <Text>Scanned data: {JSON.stringify(scanResult)}</Text>
      <Text>Render count: {count}</Text>
    </Screen>
  );
}

The count is consistently increasing even without scanning.

As a short-term workaround, I rolled out the solution from https://github.com/Shopify/ui-extensions/issues/2299#issuecomment-2310267490 a few days ago. I'm treating the hook as a singleton, i.e. providing the scan result via a top level React context (it's used in a few places). The downside is that the Shopify POS app needs to be restarted once, but after that the extension seems to work somewhat reliably - already confirmed by one of my customers. It seems to work ok on an older POS version too.

We're hoping to get it released in the next couple days (takes time to get through the app store and google), but hopefully tomorrow. @derrickrc, and anyone else who implemented the temporary hack, when this fix hits the app store we strongly recommend you revert (and test of course) to avoid any weird side effects that might happen.

Is there a nightly pre-release build we could test against in advance? Generally, it'd be useful if we could view upcoming releases and test those prior to Shopify POS rollouts to avoid similar disaster updates in the future. Of course, it would be appreciated if the testing done on your side was a bit more thorough.

js-goupil commented 2 months ago

@galmis thanks for the extra context. We are hoping to cut 9.16.2 today and have it on the app store ASAP. The fix we have clears the workers (and therefore the caches) every time the extension is unmounted, meaning that there will be no more issues with subscribables. We've have heavily tested this (hence why it's taking us a few days) to root out any problems, including with up to ten extremely heavy UI Extensions, and right now we are running smoothly. No leaks, no subscription issues, and no re-render cycles. Excited to get this out to you folks.

I'll update you once we have submitted to Apple and Google. Thank you so much once again for your patience 🙏

cc @xqwtsz @adamwooding @derrickrc

js-goupil commented 2 months ago

@adamwooding

is there more information about this new caching feature / functionality? How can POS UI Extensions take advantage of this?

Nothing for you to do on your end, POS will be pre-loading the UI Extensions for faster render times. However, we are shipping a fix to the issues which will kill the workers and therefore clear the cache everytime the extension is closed (currently the workers are running as long as POS is running, which is causing leaks and issues with subscribables), as it was prior to 9.16.0.

js-goupil commented 2 months ago

@galmis @xqwtsz @adamwooding @derrickrc Hey folks, we've just released 9.16.2 for iOS. It contains a fix to the issue, you should be able to revert your temporary fixes. Thanks again for your patience. 9.17.0 is scheduled for release next week and also contains this fix. Waiting for Google to approve the release on the Android end.

galmis commented 2 months ago

Thanks for the update @js-goupil. I don't think reverting the temporary fixes is an option until the Android release at the very least. Ideally, I'd like all merchants using the affected POS app version to upgrade first. It'd be useful if there was an API exposing the current user device's OS details and Shopify POS version.

js-goupil commented 2 months ago

@galmis It looks like the android version was just approved by Google. We've released it to 100% of merchants, so everyone with auto updates turned on should get it.

galmis commented 2 months ago

Thank you, @js-goupil!

I don't own Shopify POS GO, but I'm assuming it was impacted by the earlier update as well. Assuming that's the case, was the issue fixed in POS GO?