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

Need preset DOM or StructuredClone #2

Closed
dfahlander opened this issue Jan 15, 2017 · 25 comments
Closed

Need preset DOM or StructuredClone #2

dfahlander opened this issue Jan 15, 2017 · 25 comments

Comments

@dfahlander
Copy link
Owner

'builtin' preset only contains ecmascript types. Other types from StructuredClone algorithm (except Blob, which cannot be serialized synchronically) should be defined and available as a preset.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 9, 2017

I'm thinking that postMessage should be either altered to be the same as the Structured Cloning Algorithm or removed. It is actually not supposed to pass in Error and indeed fails if one tries to do so in Chrome. (Also, since the built-in types are no longer included in typeson by default, postMessage would currently fail to clone anything besides errors and cyclic objects now.)

I suggest having a structured cloning type instead of postMessage given that the former has wider uses, but if not, I think postMessage needs to be updated.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 9, 2017

(Btw, my recent test addition to typeson-registry will not work until the new version of typeson's master is also published.)

@dfahlander
Copy link
Owner Author

dfahlander commented Feb 9, 2017

Hmm, the 'post-message' preset name is a little misleading. It's there to complement the structured clone as structured clone has limitations for practical reasons (marshaling Error objects for example)

@dfahlander
Copy link
Owner Author

It's meant for using together with postMessage. Since postMessage() already supports structured clone, this one just need to encapsulate Error objects and leave the rest to the browser to marshall.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 9, 2017

Oh, yeah, I c... I'm thinking also from the Node perspective where we have to shim postMessage and such. So if you added DOM types, they could go in here as per this issue (as well as perhaps Proxy, Promise, WeakMap which apparently are not supposed to fit the SCA and might not be possible unless able to share state)?

Speaking of which, would you be open to a type for functions (using toString) (and possibly one for symbols)? They wouldn't be the same (and not part of SCA), but could be useful to have. Not having heard back, I gave an example of such a type (for functions) in the README but did not include.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 9, 2017

I've pushed an initial implementation of SCA... Let me know what you think...

I am probably missing some others that should throw such as any exotic objects besides Proxy or other objects with internal slots besides [[Prototype]] or [[Extensible]]. Also haven't looked around to see what actually calls IsDetachedBuffer as not called within the ES spec though defined there.

As far as my use of Array.prototype.includes, I think that should be ok since a person wishing to use DOMException as a constructor would need to have more recent browser support anyways.

@dfahlander
Copy link
Owner Author

Very nice! I've released this as version 1.0.0-alpha.1 on npm. Keeping the issue open for complementing missing types and exceptions.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 10, 2017

The cloning algorithm mentions (non-array) exotic objects as failing.

I realized I could add some more error checking (built-in exotic objects we were missing, i.e., arguments, Object.prototype, and module objects)--sorry I didn't get that in earlier...

While I think we have covered those exotics defined in the ES spec, I couldn't find explicit reference to DOM objects as being such (and thus failing the algorithm), though I did add duck-typing
to fail on DOM nodes anyways, as they do fail in Chrome and are documented on MDN as failing.

instanceof checks we use in types, e.g., ImageData, might need to check first for their existence, as the latter is not present in Node, so I added a check for that. In other cases, though, one might appreciate the tip-off of an exception that a class doesn't even exist to check against in case one wanted to polyfill it. But I think as ImageData is not like a recent addition to ES itself, I think it is different from the likes of TypedArray.

@dfahlander
Copy link
Owner Author

Good. Released it as 1.0.0-alpha.2

@brettz9
Copy link
Collaborator

brettz9 commented Feb 11, 2017

Thanks! I've elaborated on remaining questions and asked for help at http://stackoverflow.com/questions/42170826/categories-for-rejection-by-the-structured-cloning-algorithm

@brettz9
Copy link
Collaborator

brettz9 commented Feb 12, 2017

Added one more tweak with docs (but will also rely on a typeson update)...

@brettz9
Copy link
Collaborator

brettz9 commented Feb 13, 2017

And now I've removed the frame/module-dependent constructor and instanceof checks throughout typeson and typeson-registry.

I think there is a problem with building typeson-registry with Webpack now though. I'm guessing it may be due to the cyclic dependencies. I don't know much about Webpack but think browserify would be fine with this kind of thing if it is no difference to you.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 15, 2017

There is one breaking change which I think ought to be made.

The replacers do their test and replace checks in reverse order (with your while (i--) { loop) so objects will first be detected according to the last entered replacers. I think it would be more intuitive to iterate from the first entered items so that, e.g., the following would not effectively ignore the specific MyClass detection because of the generic test being run first and matching:

var typeson = new Typeson().register([
    {specificClassFinder: [(x) => x instanceof MyClass, () => {...}]},
    {genericClassFinder: [(x) => x && typeof x === 'object', () => {...}]}
]);

Would you be open to such a change? (I've documented the current behavior and added a test for it for the time being, but it can easily be reversed.)

@dfahlander
Copy link
Owner Author

My thought has been to have generic rules first. Let's say someone creates a module that extends Typeson and pre-registers a preset in its constructor. The consumer of that libraries should then be able to call register() to override general rules to more specific ones.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 16, 2017

Makes sense... But one could still keep specific-generic ordering within the array and allow subsequent calls to splice onto the beginning of the array. From a linear programmatic point of view it seems to me the specific ones in a block ought to catch first even if subsequent calls get higher priority. Of course whatever way can work.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 27, 2017

Hi,

I've pushed a new change to typeson proper which allows async encapsulation/stringification. Basically, it works by allowing type encapsulators to return a Typeson.Promise (which behaves like a promise but avoids being confused with an actual Promise, e.g., if the SCA is, as it appears, supposed to throw on one). The encapsulator resolves when it is ready, and Typeson detects this to resolve the actual promise underneath and return the result. I've added a ImageBitmap type in a branch I am working on for typeson-registry which uses this approach, and I hope to also add for Blob, File, etc. (I forgot I added this without such a need, as the only async needed was to return a Promise in the reviver)

Although I thought it would be best to have a breaking version which forced the async API at least by default (in case a type relied on async behavior) and so that insistence on using the sync API could be done via stringifySync/encapsulateSync, I was hesitant to break all existing code. I'd like to hear your feedback.

While there may be some use cases for leveraging typeson to allow types to be reported instead as events, allowing resolution and handling as received (e.g., as in a stream), to allow cancellation or pull-parsing (including of simple objects) as an object-based equivalent to clarinet (or adapting it to work with actual streams), I think the Promise-based approach I added ought to be an adequate, approach for now.

@dfahlander
Copy link
Owner Author

dfahlander commented Feb 27, 2017

Hm. It's nice you start dealing with this architectural necessity.

I've no concerns making a breaking change and bump the major version. However, for keeping the analogy with JSON.parse() and JSON.stringify() I prefer keeping the method behaviour synchronous and instead add *Async() versions to support async versions. This also indicates that the sync version of the API will continue to be a first class citizen in future and will not be deprecated.

Do we really need a custom Promise here? It's a lot of code just because we need to follow W3C's spec and throw when a standard Promise is encapsulated. Couldn't we just let the registered encapsulator function return a std Promise that we put in an internal representation with less code (the internal representation wouldn't need to follow the entire Promise API).

My suggestions would be the following. Note however, that I may have missed a point. If so, please correct or question me ;)

Synchrounous API

stringify() - synchronous, as now
parse() -"-
encapsulate() -"-
revive() -"-

Asynchronous API

stringifyAsync() - returns Promise
parseAsync() - returns Promise
encapsulateAsync() returns Promise
reviveAsync() returns Promise

Registered types

  • encapsulators and revivers may return Promise. When they do, the type will be considered asynchronic and if caller use a synchronous API, it must throw. Otherwise the returned Promise will be pushed to an array of awaited promises, a placeholder for the value will be inserted. In the end, return Promise.all(arrayOfPromises).then(results=>{replacePlaceholders(results); return result;}).

W3C Compliance

Trying to stringify a Promise should throw (if imported SCA preset). Therefore we need to have a built-in handling of whenever a registered reviver or encapsulator returns a Promise, it will be converted to an internal representation of that Promise (much like your Typeson.Promise, but won't need all the bells and wistles of it).

The sync versions should through when hitting an async type.

Future support for streams

A future version may add support for streams as well instead of promises (postfixing methods with *Stream). If it would have been possible to construct a Blob without having its entire content in memory, we would be better off with a stream-based API (based on either Async Iterables or ES-Observabe). Now as Blob cannot be constructed from a stream, a simpler API is probably Promise based.

This was referenced Feb 28, 2017
@brettz9
Copy link
Collaborator

brettz9 commented Feb 28, 2017

Thanks for the prompt and thorough response and consideration.

Good point on having async for the reviver too. I had considered this, but dismissed it initially in thinking that the user would know it was async if a promise were returned, but on further consideration, in using async, they'd be able to immediately introspect on the final resolved type, whether async or not.

I also like the idea of throwing (though perhaps optionally, see below) when encountering an async type in using a sync method.

Sounds good about stream support. I've added dfahlander/typeson#3 for tracking this.

Preference of Async over Sync

As far as keeping the analogy with JSON.parse/JSON.stringify, that makes sense.

However, I do think we ought to give some strong suggestions to users to prefer the async methods.

  1. The async methods allow for the most future-proof code, as their expected result type, or objects within their expected result type might end up relying on async, in which case, they'd need to change all of their code. With the extensible, modular system you have set up, I think "plug-and-play" between types ought to "just work" in most cases.

Besides this, the browsers are deprecating things like sync XHR for a reason I must begrudgingly admit is important, namely preventing degraded performance from sites hogging resources, so if types can be implemented with both, the async ought to be encouraged.

  1. One other item I forgot to mention is the possibility of allowing an additional encapsulator so that the implementations could differ appropriately for sync and async (e.g., doing a sync XHR for a Blob vs. an async one). We might use this in IndexedDBShim to throw for those methods which expect sync reporting of cloning errors upon encountering errors in retrieving the Blob but for which async would normally be preferred.

And in such a case where both sync and async are possible, it would also be desirable to normally encourage the async method for UX reasons.

Typeson.Promise

The uses for a special Promise type are not limited to the SCA. It is conceivable that users may have objects which resolve to, or more likely, have parts resolving to Promises yet they do not mind these being converted to plain objects by JSON stringification (e.g., to just ignore those parts).

To future-proof the mechanism within Typeson, I really feel we ought to provide such magic values as Typeson.Promise for our own purposes without fear of conflict with user values, whatever the user might be trying to do. (For one remaining area, see dfahlander/typeson#2 )

(Though if you agree to keeping this, I indeed ought to add a (default) option to throw when a sync call is used and a Typeson.Promise is returned.)

As far as the extra code of Typeson.Promise, we don't actually technically need all of the bells and whistles I added. There are two considerations in this regard:

  1. The following should be all we need in the future when ES6 classes are more widely supported:
class TypesonPromise extends Promise {
    get [Symbol.toStringTag]() {return 'TypesonPromise';}
}
  1. I didn't want to disappoint consumers who might hope for a similar behavior to regular Promises. If you wanted to cut it down to the bare essentials, however, this is the only part we'd technically need:
function TypesonPromise (f) {
    this.p = new Promise(f);
};
TypesonPromise.prototype.then = function (onFulfilled, onRejected) {
    var then = this.p.then.bind(this.p);
    return new TypesonPromise(function (res, rej) {
        var rejected = false;
        var result = then(onFulfilled, function (r) {
            rejected = true;
            return onRejected(r);
        });
        rejected ? rej(result) : res(result);
    });
};

(Note: I do still need to test rejections which I might not be properly handling yet and which could possibly necessitate a bit more code. push my changes to your repo.)

@brettz9
Copy link
Collaborator

brettz9 commented Mar 8, 2017

I've pushed a bunch of changes to typeson including tests/fixes for some edge cases and the API you proposed with the following changes/additions (which I can adjust if they are not suitable to your liking):

  1. I kept Typeson.Promise though I refactored to better minimize the code. If you're really dead-set against this, I would hope to at least allow a custom Promise implementation be allowed to be passed in. But in my view, including it makes for the most robust and future-compatible code among various types.
  2. In addition to adding the *Async methods and keeping the stringify/parse/encapsulate/revive sync methods, I added *Sync methods too (adding just a few lines of additional code). I added these so that, as with the *Async methods, they could throw on encountering a bad sync type, while allowing for the regular stringify/parse/encapsulate/revive methods to be more flexible.
  3. I allowed for a purely object-based API, along with checking for replaceAsync and reviveAsync if in async mode since it is possible, e.g., for Blob to desire either a sync or async implementation.

Would be glad to get your feedback.

But even more urgently, from my perspective, I found a problem (which pre-existed my changes) for a certain type of recursive array (possibly objects too) for which I would particularly appreciate any help: dfahlander/typeson#4

@brettz9
Copy link
Collaborator

brettz9 commented Mar 8, 2017

Oh, and I just pushed changes to typeson-registry utilizing the new typeson changes along with other fixes and enhancements (including browser testing via browserify) and File, Blob, FileList, and ImageBitmap types.

@dfahlander
Copy link
Owner Author

Ok, thanks! I'm not dead-set against the Promise thing. I'm ok with that. Having two sync APIs may confuse the user though. Is it really nescessary to have a *Sync version? Couln't you do those checks in the non-postfixed versions?

@brettz9
Copy link
Collaborator

brettz9 commented Mar 9, 2017

No, I can drop the *Sync versions if you like. I'd at least like to keep the (non-default) option not to throw upon encountering a different sync/async result though (e.g., if you wanted a sync result to get at some data but didn't care if unresolved promises (for async subtypes) were still elsewhere within the result, or, on the contrary, were requesting an async result where, even with some async types, it is natural some sync types could be mixed in that one would want resolved).

@brettz9
Copy link
Collaborator

brettz9 commented Aug 13, 2020

I think this can be closed now? The Stream API concept discussed in this thread has an issue already at dfahlander/typeson#3 .

@brettz9
Copy link
Collaborator

brettz9 commented Nov 11, 2023

I'll go ahead and close, but feel free to reopen if there is something missing here, @dfahlander

@brettz9 brettz9 closed this as completed Nov 11, 2023
@dfahlander
Copy link
Owner Author

Go ahead! Sorry for missing out to respond

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

No branches or pull requests

2 participants