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

Add support for nested method structures #71

Merged
merged 1 commit into from
Jul 29, 2021

Conversation

cdun
Copy link
Contributor

@cdun cdun commented Jul 27, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

cdun commented Jul 27, 2021

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
Copy link
Owner

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
Copy link
Owner

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 Aaronius merged commit c0d646a into Aaronius:master Jul 29, 2021
@Aaronius
Copy link
Owner

Published in 6.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants