Aaronius / penpal

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

Serializing Objects with Functions causes an error #56

Closed OverZealous closed 3 years ago

OverZealous commented 3 years ago

This isn't too hard of an issue to workaround, but it might be nice to handle in the core library.

Problem

If you pass an object to postMessage that contains a Function, the method throws an error (at least under Firefox).

Solution

This can be handled very simply using a method that filters out functions from passed data. I use a method that looks like this:

export function toSafeObject(data) {
    let newData = data;
    if(typeof data === 'function') {
        newData = undefined;
    } else if(Array.isArray(data)) {
        newData = data.map(toSafeObject);
    } else if(data !== null && typeof data === 'object') {
        newData = {};
        Object.keys(data).forEach((k) => {
            newData[k] = toSafeObject(data[k]);
        });
    }
    return newData;
}

I'd be happy to open up a PR with this change in place, along with some tests.

Aaronius commented 3 years ago

Thanks for logging the issue @OverZealous. I think I prefer the current behavior of an error being thrown, so that it's more obvious to the developer that they can't send a function to/from the iframe. I do think I need to be more explicit about this nuance in the Penpal documentation though.

FWIW, I think you can change your function to this:

export function toSafeObject(data) {
  return JSON.parse(JSON.stringify(data));
}

The native JSON serialization process automatically strips functions. I'm going to close this issue, but feel free to continue to comment or add questions.

OverZealous commented 3 years ago

I understand. I wish the browser-generated console error was better—it's not very clear what's triggering it.

One reason it would be nice to automatically handle this is you don't have to wrap methods or results from methods that might be shared within the parent or child application. This is useful for triggering actions that can be called on either side by simply sharing the same method.

I did look at the stringify method before, but I believe it strips out values that are valid to send across postMessage. I think you can serialize several data types that aren't supported within JSON, such as NaN, Infinity, Symbol, Errors, and the various *Array objects like Int8Array. Though admittedly, my method isn't that robust against the arrays and friends.

I agree It's not critical enough to add to the library. However, would it make sense to add serialize & deserialize options to the library? This would allow the user to clean up any data in a concise manner.

Aaronius commented 3 years ago

Thanks for pointing that out. Let me keep this in mind. If it comes up again, it might be worth adding serialize and deserialize options. Thanks again.

sgarfinkel commented 2 months ago

Sorry to necro this, but I think it would be handy to update the type for Methods to properly constrain this, like the below snippet (using type-fest since it already has Jsonifiable which captures all the serializable types)

import type { Jsonifiable } from 'type-fest';

type SerializableFunction = (...args: Jsonifiable[]): Promise<Jsonifiable> | Jsonifiable

type Methods = Record<string, Methods | SerializableFunction>;