Aaronius / penpal

A promise-based library for securely communicating with iframes via postMessage.
MIT License
381 stars 56 forks source link

Add support for nested method structures #71

Closed cdun closed 3 years ago

cdun commented 3 years ago

Whilst using the library I ran into the same limitation that a few other people seem to have found with the methods object only being able to support a flat structure. Searching through the issues, I see this was requested in #12 but seems to have stalled.

This PR adds support for nesting at arbitrary depths in the exposed object, copying over all functions that are directly present on it (that is, not from the prototype). It does this by serialising methods as a map from key path to value in the source object - these key paths are later unpacked onto the call sender, recreating the structure provided. The included updates to the typings maintain the existing type safety for nested objects whilst also exposing any functions found in child objects. All considered, this should therefore be a backwards compatible change.

Aaronius commented 3 years ago

Thanks for the PR @cdun. Do you mind providing more details about your use case? Particularly, I'm interested in how inconvenient it is for your workflow to have a flat object of methods.

cdun commented 3 years ago

You bet! I'm building out a relatively large API comprised of something like 20-30+ methods (maybe more in future), exposed to a child via PenPal. The final shape of the exposed API mixes in these methods with some auto-updated data properties that are come from the parent. To keep things simple for end user, each slice of functionality in the API is grouped by feature. In pseudo-types, our current desired API looks something like:

// Data that is automatically updated in the child by the parent.
interface Data<T = any> = {
  listen: (updatedValue: T) => void;
};
// Any method implementation for example sake
type Method = () => Promise<any>;

interface PublicAPI {
    feature: {
        activeCount: Data<Number>;
        doSomethingInParent: Method;
    },
    anotherFeature: {
        activeCount: Data<Number>;
        toggleThingInParent: Method;
        doSomethingElseInParent: Method;
    },
        // ... lots more ...
}

// Example usage with the a fully hydrated PublicAPI:
const { feature, anotherFeature } = await connect();
feature.activeCount.listen(activeCount => {
  if (activeCount > 10) {
    anotherFeature.toggleThingInParent();
  }
});

We could expose a flat API by prefixing each item, such as:

interface PublicAPI {
  featureActiveCount: ReactiveData<Number>;
  featureDoSomethingInParent: Method;
  anotherFeatureActiveCount: ReactiveData<Number>;
  anotherFeatureToggleThingInParent: Method;
  //... etc
}

However, having a flattened API like this would put terms that look passing similar, but are in reality unrelated, in close proximity to each other lexically. It would also likely result in otherwise unnecessarily long method names to ensure clear namespacing. With sensible nesting and with the ability to destructure keys that contain related functionality, the API is (subjectively!) more ergonomic and hopefully easier to grok for our target users.

We could in theory do something like this in user space with the primitives that PenPal provides, serialising the methods object before it reaches the library, especially given that we're already doing so to expose data. Having seen the linked issue and your previous comments - which I appreciate are now 4 years old 😅 - it felt like this might be a feature worth implementing in PenPal itself.

Aaronius commented 3 years ago

Thanks for explaining @cdun. It makes sense. I'm open to it. I'll look over your changes as soon as I get a chance.

Aaronius commented 3 years ago

This looks good. It increases the file size of penpal.min.js by 8% (from 6671 bytes to 7223 bytes), but I think that's probably reasonable. Thanks for the contribution!

Aaronius commented 3 years ago

Published in 6.1.0.