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

Different behavior for method collision #45

Open
anticore opened this issue May 21, 2018 · 23 comments
Open

Different behavior for method collision #45

anticore opened this issue May 21, 2018 · 23 comments

Comments

@anticore
Copy link

Greetings, stamp people.

I've been messing around with stamps for a relatively small project and it has been a fantastic development experience.

That said, I'm interested on how to implement a different behavior for method collision.

As far as I understand, collision defer calls the colliding methods sequentially. What I would like to be able to accomplish is to pipe the return value of each colliding method to the next.

For example, if I wish to have a 'stamp-wide' toJSON() serializer, I would like each of the composing stamps to have a toJSON() method. If the toJSON() methods were expecting a previous value, that would considerably simplify the complexity of such a feature.

Something like:

const Stamp1 = stampit({
    props: {
        name: "stamp1"
    },

    methods: {
        toJSON(previous) {
            const _json = { name: this.name };
            return !previous ? _json : {...previous, _json};
       }
});

Does this make sense? Is there an already established way to accomplish something like this?

Thanks in advance.

@koresar
Copy link
Member

koresar commented May 21, 2018 via email

@anticore
Copy link
Author

@koresar I'm not quite sure... Is it good practice for the stamps to be 'aware' they'll be used with a collision stamp? If so I don't see a problem with explicitly declaring the methods with a first argument to expect the return value of any previous colliding methods (see example above). I'm not sure how to handle 'dumb' stamps though.

I might take a crack at adapting the collision stamp, but I'm looking forward to see what you can come up with.

@koresar
Copy link
Member

koresar commented May 22, 2018

To fix the 'awareness' concern you can "inherit" (aka compose with) from your collision stamp.

const DeferToJSON = Collision.collisionSetup({ defer: ["toJSON"] });

const Stamp1 = stampit(DeferToJSON, {
  props: ...
  methods: {
    toJSON() { ...

Now the stamp is aware it has a deferred "toJSON" method.


Semi good news. I have just published @stamp/collision v1.1.0. Now if you "defer" a method it will return an array of method returns.

const MegaStamp = stampit(Stamp1, Stamp2, Stamp3, Stamp4); // each stamp have "toJSON" method

const results = MegaStamp().toJSON(); // array of 4 items
const json = Object.assign({}, ...results);

That's not exactly what you need, but it works today.


The functionality you ask is called "pipeline". It's not as quick to implement as I thought. I need some more time.

@anticore
Copy link
Author

@koresar That definitely works for now. Thank you for your quick response and implementation. Interested to see how you solve the pipeline problem.

@koresar
Copy link
Member

koresar commented May 26, 2018

Taking a second look I realise that API of this stamp could be better. More flexible and more featureful. For example, I always wanted to protect props and static methods collision - not just methods.

How about this API:

import {collisionSetup} from '@stamp/collision';

const PipeToJSON = collisionSetup({ methods: { toJSON: "pipe" } });
const DeferToJSON = collisionSetup({ methods: { toJSON: "defer" } });
const ForbidCollisionToJSON = collisionSetup({ methods: { toJSON: "forbid" } });

const ForbidCollisionPropFoo = collisionSetup({ properties: { foo: "forbid" } });

@anticore
Copy link
Author

Looks good.

Can you see properties being able to use the pipe collision handler? For example, arrays being concatenated, objects deep-merged, etc. Not sure about primitives.

@koresar
Copy link
Member

koresar commented May 28, 2018

I do not see regular properties piped or deferred. We have deepProperties for array concatenation and object deep merging.
https://stampit.js.org/api/deep-properties

@koresar
Copy link
Member

koresar commented Jun 16, 2018

I'm working on it. Rewrote the whole thing 3 times now. Found a best solution. It's coming

@koresar
Copy link
Member

koresar commented Sep 1, 2018

I'm close :)

@koresar
Copy link
Member

koresar commented Sep 1, 2018

The PR is ready: #48

@sammys
Copy link
Contributor

sammys commented Mar 24, 2021

Hi there stampers. Since this is my first post to @stamp I'd like to thank everyone for making this suite of packages! Utterly fantastic! Let's get down to business hehe

I've started working on a project using stamps and several of the packages require some rather fancy collision handling. I ended up almost completing a reimplementation of @stamp/collision and then stumbled upon this issue. What I implemented might be interesting to others so I thought I ought to mention what I did.

First up, I felt defer wasn't really describing what was going on so I renamed it to map so it's more obvious what it does. I also implemented reduce, which is equivalent to pipe as described in this issue.

Second of all, I implemented async versions of both. I'm heavily using reduceAsync in my packages to properly handle asynchronous initializers though that was via an additional setting initializersReduceAsync. Even though that is working well I think the new implementation in #48 is a better solution and, as you might imagine, I'm rather glad I stumbled on to this issue.

So, I'm about to start merging what's been done in #48 and what I've done with naming and async support so I can get the full awesomeness for my project. Are there any concerns about the naming or anything else that anyone would like to discuss and is there any interest in having it as a PR?

@sammys
Copy link
Contributor

sammys commented Mar 25, 2021

Update: After merging what I have with the implementation in master all tests are passing for the map setting (formally known as defer) for methods using this type of syntax:

{ methods: { map: ['draw'] } }

I've decided to use the following names for the settings:

  • map: returns an array of values
  • reduce: returns a single value and whatever is bound to this will be the starting value for reduce
  • mapAsync: returns an array of Promise
  • reduceAsync: returns a single Promise and whatever is bound to this will be the starting value for reduce

In #48 there was use of the word aggregate so that's what I've used to designate the above settings

I also had to tweak compose a little bit so it would play better with the strict index signature requirements you have in place

@koresar
Copy link
Member

koresar commented Mar 26, 2021

Hi @sammys

I'l reading every word you write :)
Thanks for the work!

I myself couldn't finish the coding of the feature. Maybe you could? I can give you write access to this repo. Do you need it?

I'll be happy to accept any design/update you feel is better. So, yeah, a PR would be great. :)

@sammys
Copy link
Contributor

sammys commented Mar 26, 2021

Hi @koresar ... I appreciate the kind words. I'd be happy to contribute some time and push stamps forward a little more. Perhaps we can discuss details somewhere else since that is OT?

Update: I've completed implementation of all four aggregation types (map, reduce, mapAsync and reduceAsync) for methods. I fixed what I consider to be a bug (or limitation) in the current released implementation also.

The released implementation requires that every subsequent composition containing a Collision-setup stamp requires the Collision to be setup again in the new composition. I believe the original intent of this stamp was that the Collision setup survives future compositions so I fixed it. Please let me know if this was not the original intent.

I have to add a few more tests and then I'll push a branch to my fork for anyone wanting to weigh in on the changes.

@koresar
Copy link
Member

koresar commented Mar 31, 2021

You can reach me in twitter DMs if you believe that's a better communication channel.

I'm waiting for you to tell me what to do. :)

@sammys
Copy link
Contributor

sammys commented Mar 31, 2021

Screen Shot 2021-03-31 at 21 35 30

Both the implementation and tests were written in Typescript. I've migrated tests to JS and run jest with the above result. Now that I've confirmed that the migration is sound I'll push it up to a new branch on my fork.

@koresar
Copy link
Member

koresar commented Mar 31, 2021 via email

@sammys
Copy link
Contributor

sammys commented Mar 31, 2021

I hope the TS would stay as the primary language.

Yeah. Typo. I migrated the tests to JS. Updated the post.

@sammys
Copy link
Contributor

sammys commented Mar 31, 2021

Let me know what you think. I haven't made any changes to README.md yet awaiting approval of this direction and any honing that is to be made before it is worthy of master.

@sammys
Copy link
Contributor

sammys commented Apr 2, 2021

Now that we have the new settings format available I've thought of one other way we can expand the functionality of @stamp/collision a little more once #80 is merged. We could introduce a way to force particular aggregated methods to be executed either before or after the others. Here's an example to illustrate:

Let's say you have stamps A and B with each having an initializer (initA and initB). They are implemented in different third-party packages so neither are aware of the other in their code.

Along comes Johnny A. Developer and composes stamp D1 with A and stamp D2 with B and then they all get composed into stamp BIG: compose(compose(D1, A), compose(D2, B)).

Let's now put in a requirement that just the initializers initA and initB have to run after all other initializers in the BIG stamp or any other stamp that BIG is composed into.

To make this happen we could add a setting in the initializers domain. E.g.
{ initializers: { forceLast: [ 'initA', 'initB' ] } }

What this does is push initA to the end then push initB after that. Any thoughts?

@koresar
Copy link
Member

koresar commented Apr 4, 2021

I gave a deep thought to Promise returned from initialiser. Here are my thinking.

  • Stamps are alternative to classes. One can't have async constructor in any of the languages.
  • Stampit used to handle promises returned from initialisers. It was a mistake back then - code got significantly complex, users have never adopted the feature. We removed the promise support from stampit.

Sorry I'm coming too late. But I suggest to implement Promise support some day later in a separate package.

@koresar
Copy link
Member

koresar commented Apr 4, 2021

I'm looking at the PR and...
Maaate, this is so neat! Loving the unit tests especially! So good. So perfect!

@sammys
Copy link
Contributor

sammys commented Apr 4, 2021

I gave a deep thought to Promise returned from initialiser. Here are my thinking.

  • Stamps are alternative to classes. One can't have async constructor in any of the languages.
  • Stampit used to handle promises returned from initialisers. It was a mistake back then - code got significantly complex, users have never adopted the feature. We removed the promise support from stampit.

In the project in which I've used Stamps, all initialisers are async because the Stamps (and any they are composed into) need to be capable of performing async processing during construction to simplify code. I implemented what you now see in the PR to support these. There were no terrible side-effects and also no complexity beyond what is normal for async code.

After seeing the simplification it enabled first hand I now believe the OOP limitation sucks and is something Stamps overcomes and thus gains yet another major advantage.

I'd love to hear what the problems were with async. Can you point me to sample code or a commit hash to checkout?

I understand that the stamp specification currently does not support a promise returned from an instance and I will remove the Promise<> type declaration. That said, the implementation in the PR still works

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

3 participants