Draft: Add support for iframes

[karthikiyengar]

Per discussion here, I've been hacking around trying to get iframe support up and running.

What works (with multiple iframes on a page)

• Inspecting Queries
• Inspecting Mutations
• Cache Explorer

What's pending

• Explorer: I haven't figured out a way to get it to use the correct Apollo Client yet. It always prefers the window. Perhaps this could be a separate PR to keep scope in check.
• Generating a better ID for windows and iframes. Currently using a cuid, but this should ideally be something intelligible like the native browser frame selector. Work is also needed on UX here.
• Removing stale iframes from the list if they dismount: I don't know what it is, but emitting a postMessage on onunload/pagehide doesn't seem to be picked up 🤷‍♀️. I guess we can just let them remain in the list, but it would be nice if it auto-updates. Convoluted solutions with beacons might be possible, but the cost/benefit doesn't make sense.
• Fix existing tests
• More coverage for new functionality

Demo

I have the test app running, and another create-react-app with a single query running in a setInterval to simulate two clients running on different frames. Here's a video:

apollo-multi-iframe-demo-small.mp4

I also tried out a sample app on Codesandbox and it seems to work fine. Note that sometimes CodeSandbox may take more than 10 seconds to initialise, and the connection won't be established in those cases.

Since this has turned into a relatively large refactor, I'd like to get some early feedback on the work. CC: @bignimbus @jpvajda @MrDoomBringer (apologies for the direct tag, didn't find a better way to page maintainers)

[apollo-cla]

@karthikiyengar: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[jerelmiller]

Hey @karthikiyengar 👋 ! Thanks for the contribution!

I'd be happy to review this PR, though just to set expectations, I'm finishing up a few commits before the holidays. I can't guarantee I'll be able to get to it before the New Year, but I'll do my best to review it shortly after. Thanks so much for the help!

[jerelmiller]

Related to #380

[karthikiyengar]

@jerelmiller - Polite bump in case this slipped between the cracks. Just wanted to figure out if the team would be open to this change, so I could prioritise better. It would be a pretty big win for @Shopify app devs (and other Apollo users who work with iframes).

Aside: Would you please mind committing an .editorconfig to the repo? Vscode's default formatter causes unnecessary diffs on my machine.

[karthikiyengar]

Extracted to prevent a cyclic dependency in Cache.test.tsx

[jerelmiller]

Hey @karthikiyengar 👋 !

Apologies, I got caught up on some other work! I'm working to figure out how to get this project setup myself as this is the first I've worked with this repo. I'm hoping I can get it setup soon so I can take a look at this PR in context. Thanks for you patience!

[karthikiyengar]

Suggestions welcome for a more intelligible (and unique) frame identifier.

[karthikiyengar]

Hey @karthikiyengar 👋 !

Apologies, I got caught up on some other work! I'm working to figure out how to get this project setup myself as this is the first I've worked with this repo. I'm hoping I can get it setup soon so I can take a look at this PR in context. Thanks for you patience!

Hi @jerelmiller. Don't want to sound pushy, but it'd be great if we can get a signal around whether this could potentially be upstreamed. I'd really want to avoid a fork if I can.

I'm happy to set aside time to pair/help setup if you think that would help.

[karthikiyengar]

@jerelmiller - We've currently forked this, upgraded the extension to Manifest v3, and have released this internally for now.

In case the maintainers are still open to pulling the changes upstream, I've created #997 that's in sync with current main.

Before we decide to release this publicly, please consider another appeal to potentially upstream this into the extension. This would help us avoid fragmentation, and address a long-standing feature request.

CC: @benjamn @alessbell @bignimbus @hwillson @jerelmiller @phryneas

[phryneas]

Before we decide to release this publicly, please consider another appeal to potentially upstream this into the extension. This would help us avoid fragmentation, and address a long-standing feature request.

We are still open to these changes but are a small team working on many things simultaneously.
This big PR needs a thorough review, testing, and additional work from our side (for example, we cannot just update the manifest to v3, as that breaks Firefox - so we will need to set up a multi-manifest setup).
We will get to this, but please be patient (and, if possible, do not publically fork this in the extension store) while we also work on the release of Apollo Client 3.8 (and many other things).

[karthikiyengar]

We are still open to these changes but are a small team working on many things simultaneously.

Thanks for the clarification.

We will get to this, but please be patient (and, if possible, do not publically fork this in the extension store) while we also work on the release of Apollo Client 3.8 (and many other things).

Totally understandable and happy to wait.

[jerelmiller]

@karthikiyengar I do have to apologize. I think I was a bit too ambitious when I first responded to this PR.

As I started looking at your changes above, I realized there is a lot I had to learn about the current setup of the extension so that I could best understand the changes being proposed here (I was new to this repo when I first responded). In my effort to start understanding how this extension is put together, I've opened a few PRs to upgrade deps, add a small feature, etc. This has helped me at least grasp some of how the repo is put together so that I could more intelligently understand the proposed changes. As @phryneas mentioned, it benefits us to do a thorough review, including testing the changes, which obviously means setting time aside not just to look at the changes in isolation, but to understand how they fit into the whole picture.

I'm currently working on adding React Suspense support for v3.8 which has taken the majority of my time. The changes for the extension have been a "take a bit of time here and there" approach, but this isn't obvious to someone not sitting in the room with me. Apologies I haven't been super communicative on this PR as I know you've been patiently waiting.

Since you said you've forked it, feel free to consider privately using that fork for the time being. I promise we WILL look at this, I just can't guarantee a time frame until we are a bit further along in 3.8 and I've had a chance to play around with the extension a bit more.

Thanks for your understanding, and again, I apologize for not communicating as much as I should have!

[shanesc]

@jerelmiller any updates on this?

[djkgamc]

@jerelmiller - We've currently forked this, upgraded the extension to Manifest v3, and have released this internally for now.

In case the maintainers are still open to pulling the changes upstream, I've created #997 that's in sync with current main.

Before we decide to release this publicly, please consider another appeal to potentially upstream this into the extension. This would help us avoid fragmentation, and address a long-standing feature request.

CC: @benjamn @alessbell @bignimbus @hwillson @jerelmiller @phryneas

I'd be interested iny our public release

[karthikiyengar]

I'd be interested iny our public release

Sorry, I haven't touched the code in a while, and I'm not using Apollo actively. For what it's worth, you should still be able to compile it and install it as an extension from developer tools without too much trouble.