Collision: Expand defer into map/reduce, add support for initializer aggregation, aggregating asynchronous methods and be backwards compatible.

[sammys]

For #45. Will add details in here if need be.

[sammys]

Looks like CI is failing because I bumped @typescript-eslint/eslint-plugin and @typescript-eslint/parser to version 4 (latest).

Version 2 (used in master) doesn't support no-shadow so I had to bump it for eslint to have no complaints. Might be able to bump master to version 4 without any dire consequences.

[koresar]

Hello @PopGoesTheWza This is very great PR. I'd like to merge it. But before, I want to make sure it won't break (much of) your current work?

[PopGoesTheWza]

@koresar should be good.

[koresar]

Great! I'll test the PR on my laptop (when it's not midnight, which is now).

[sammys]

That last commit wraps up support for methods and initializers with all tests passing. There are a few parts that could probably be written cleaner but I don't see those as blockers.

[sammys]

I have half implemented some more tests for the newly exposed methods on the Collision stamp and some exception throwing in those methods when things aren't right. Will push those up ASAP.

[PopGoesTheWza]

@koresar any advise on how to integrate this PR into #78 ?

[PopGoesTheWza]

@sammys can you add the updated package-lock.json file to this PR? TravisCI is confused without it...

[sammys]

@PopGoesTheWza That explains it. I've pushed package-lock.json with the completed implementation of *Aggregates() methods on Collision stamp.

There are still errors in other @stamp packages so we're not out of the woods yet.

[koresar]

I have couple of suggestions.

1. Start this branch of this unfinished one: https://github.com/stampit-org/stamp/tree/typescript-PaRt-two

   • We would have no conflicts merging the two.
   • The types are more mature in there, so might not need to do as many compose.ts changes. @PopGoesTheWza Are you okay with that idea? @sammys Is this much to ask?

2. I'm giving @sammys write permissions to this repo right now. :) See email.

[sammys]

Does this PR requires the README documentation to be updated?

I will be updating the README as soon as I get an all clear for the implementation.

[sammys]

I believe that last commit covers all the change requests. Let me know if I missed something.

Are there any remaining concerns that I need to address before updating README?

[PopGoesTheWza]

Thanks @sammys. This PR is on a good track.

Last commit fails on build. Pro tip, run a npm install from the root of your local repo to check before commit.

EDIT

The build error are cause from the changes in compose/index.ts. My advise is to revert this file to original state. Next address any type issue locally inside collision/index.ts. When unsure how to correctly address a type conflict, add a // FIXME comment or 'force cast' type with (someVar as unknown) as SomeType

[sammys]

There are build issues in other packages but I didn't think those are related to this PR. I'll sort it out

[sammys]

Had to bump Typescript to >= 3.8 so dependencies can build.

Not sure if this has implications outside the package scope... switched this?.compose to this for the following two lines:

If there are implications then I will put some tests in place and fix the code.

Had to fix a repeated type declaration in shortcut

... and fixed the privatize-plus-collision test that used the old collision settings syntax

[sammys]

The build error are cause from the changes in compose/index.ts. My advise is to revert this file to original state. Next address any type issue locally inside collision/index.ts. When unsure how to correctly address a type conflict, add a // FIXME comment or 'force cast' type with (someVar as unknown) as SomeType

@PopGoesTheWza Sorry, I saw your edit after I had committed my changes already. Only one of the errors was caused by the changes in compose/index.ts. I can still revert if you want it that way.

[koresar]

I have yet told you @sammys that the current master is not what's published to NPM. What happened is - I accidentally merged @PopGoesTheWza 's half baked PR. Silly me. I hoped that there will be no other work on this repo while @PopGoesTheWza is in progress. But here comes @sammys and adds a mind blowing super functionality WITH TESTS!

The current .js file are what's actually published to NPM.

I don't know what it means, but I thought you need to know this Sam.

I've added few more comments. Some important, but most are minor.

In the next message I'll explain what's wrong with async initialisers.

[koresar]

The async initializers were pretty difficult to implement in accordance with the specification. Here is my code from 5 years ago before the Spec was born: https://github.com/stampit-org/stampit/pull/133/files#diff-30f7af44dd75ee293d1a3bdab338b6604a66ce8cbc1f666444d6c982f0c7eb5dR128-R177 It's from "before async/await" times.

That little code disallowed inter-stamp operability/composability. Stamps declare that you can mish-mash anything and get your object instances from it. Which become untrue.

What I see in your current implementation:

• All initialisers will be called, even if first of them throws. - Against specs.
• Your code passes your unit tests, but won't pass more comprehensive unit testes (see link above).
• Since your code overwrites the existing Stamp.compose.initializers to the aggregated one that stamp won't work with a number of other stamps from this very repo. E.g. this one, or that one, and especially this popular one.

Now, don't get me wrong, I really want to add async initialiser to stamps! This would be a very very very "class-beating" argument! Let's add async initiliazers in a more thought through manner we do here - discuss, update specs, update check-compose, release the new feature!

I have a great idea how we can extend the specification to your needs! Add a text like this:

If one of the intializers is detected to be an async one then the whole object instantiation becomes async.

Meaning we would detect if at least one initalizer is async

const isAsyncfunc = f => f.constructor.name == 'AsyncFunction'

If yes - then return a new Promise() which iterates over the initalizers.

Let me quickly hack through this piece of code. See my comments below.

    if (initializers && initializers.length > 0) {
      let returnedValue: void | object;
      const args = [options, ...moreArgs];
      for (let i = 0; i < initializers.length; i++) {
        const initializer = initializers[i];
        if (isFunction<Initializer>(initializer)) {

          // New code
          if (isAsyncFunc(initializer) return new Promise(async resolve => {
            for (; i < initializers.length; i++) {
              const initializer = initializers[i];
              if (isFunction<Initializer>(initializer)) {
                  returnedValue = await initializer.call(instance, options, { // await !!!
                  instance,
                  stamp: Stamp as Stamp,
                  args,
                });
                if (returnedValue !== undefined) instance = returnedValue;
              }
            }
            resolve(instance);
          });

          returnedValue = initializer.call(instance, options, {
            instance,
            stamp: Stamp as Stamp,
            args,
          });
          if (returnedValue !== undefined) instance = returnedValue;
        }
      }

Looks like (not sure though) this way we won't break stamp interoperability and will be new spec compliant.

WDYT?

[koresar]

Or even better. Do not rely on semi-stable isAsyncFunc(). And don't rely on global Promise variable.

    if (initializers && initializers.length > 0) {
      let returnedValue: void | object;
      const args = [options, ...moreArgs];
      for (let i = 0; i < initializers.length; i++) {
        const initializer = initializers[i];
        if (isFunction<Initializer>(initializer)) {
          returnedValue = initializer.call(instance, options, {
            instance,
            stamp: Stamp as Stamp,
            args,
          });

          // New code
          if (isFunction(returnedValue?.then)) return returnedValue.then(asyncReturnedValue => {
              for (i++; i < initializers.length; i++) {
                const initializer = initializers[i];
                if (isFunction<Initializer>(initializer)) {
                    asyncReturnedValue = await initializer.call(instance, options, { // await !!!
                    instance,
                    stamp: Stamp as Stamp,
                    args,
                  });
                  if (asyncReturnedValue !== undefined) instance = asyncReturnedValue;
                }
              }
              return instance;
          });

          if (returnedValue !== undefined) instance = returnedValue;
        }
      }

[koresar]

Also, I found the commit where we dropped Promises support. Here it is: https://github.com/stampit-org/stampit/commit/d639cb3bf72fa82452c4381f9caa65d3e16276c4#diff-30f7af44dd75ee293d1a3bdab338b6604a66ce8cbc1f666444d6c982f0c7eb5dL125 Turns out when the group of people drafted the specification (for stampit v3) they deliberately omitted thenables support.

But there is nothing that can stop us now! :)

[koresar]

Here was another query from someone to add async initialisers support: https://github.com/stampit-org/stamp-specification/issues/120

[koresar]

Here is an opinion on why async initialisers are bad: https://github.com/stampit-org/stamp-specification/issues/109#issuecomment-259849020

[koresar]

Last comment for today. I've created this RFC: https://github.com/stampit-org/stamp-specification/discussions/129

[sammys]

@koresar Thanks for digging all that up for me to look through. I'll go through it with a fresh mind. While the mind is not fresh I'll get this PR ready hehe.

Oh... and... Having async initialiser support inside compose would be WAY better than it being in collision!

[sammys]

What I see in your current implementation:

• All initialisers will be called, even if first of them throws. - Against specs.

Most recent commit has new tests for sync and async initialisers throwing at beginning, middle and end of the chain. I changed nothing in the collision/index.ts implementation. Any throw (or rejection, if the developer uses that syntax) at any point in the chain will prevent the subsequent initialisers from being executed and a promise rejection is issued to the calling code.

• Your code passes your unit tests, but won't pass more comprehensive unit tests (see link above).

Would be interesting to me from an academic standpoint to see just how many of those tests fail.

• Since your code overwrites the existing Stamp.compose.initializers to the aggregated one that stamp won't work with a number of other stamps from this very repo. E.g. this one, or that one, and especially this popular one.

It does intentionally break those other packages. I might be totally off base here... I see collision as a utility stamp right now. Due to that perspective the implementation in this PR is intended to be an aggregation behaviour and not a core async solution. As such, I included the code as a temporary solution for those needing the behaviour before the specs etc reach maturity. Although, related to those packages you listed, there's a little more reasoning behind my madness...

In my limited Stamps experience, ordering initialisers (or methods for that matter) in a stamp's reducer became icky when many stamps having this behaviour are composed together. E.g. many stamps expecting "mine goes first". Which one actually does go first? Yes, this can be resolved with a reducer in each level of composition. It can't be built into compose because AFAICT it can't rely on composition order in many cases because it is application specific. When faced with this conundrum I asked myself the question, is a declarative style (a.k.a collision managed) better than the reducer style (a.k.a standard compose)?

What I concluded was that the latter (standard compose) is currently bound by composition ordering and the former (collision) is independent of composition ordering provided collision's reducer is always last. My experience is so limited that I can't extrapolate all this into "we can do composition order independent declarative tweaking this way using the current Stamp specification". Or, perhaps, the specification can be extended to support this.

Now, don't get me wrong, I really want to add async initialiser to stamps! This would be a very very very "class-beating" argument! Let's add async initiliazers in a more thought through manner we do here - discuss, update specs, update check-compose, release the new feature!

I'm looking forward to helping make this happen! In the meantime, since it is a project goal to support async initialisers, having the PR async initialiser behaviour published either in @stamp/collision (or in another official stamp) would give people an officially supported way to play with async initialised Stamps so they can feel the power! @koresar it's your call!

[sammys]

The async initializers were pretty difficult to implement in accordance with the specification.

What I see in your current implementation:

• All initialisers will be called, even if first of them throws. - Against specs.
• Your code passes your unit tests, but won't pass more comprehensive unit testes (see link above).

I've hit one of these more complex scenarios in the project I'm working on and, as you expected, it does fail. In the current PR implementation the async reduce doesn't properly handle when an initialiser returns non-undefined. I've implemented a fix that appears to handle it properly. While it does run properly in the project it fails tests for some very bizarre reason and that's why I haven't yet committed anything to the PR. Working on it.