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

Promise Branching #18

Closed
Mike-Dax opened this issue Apr 14, 2020 · 1 comment · Fixed by #19
Closed

Promise Branching #18

Mike-Dax opened this issue Apr 14, 2020 · 1 comment · Fixed by #19
Assignees

Comments

@Mike-Dax
Copy link
Contributor

Hello,

One of the powerful features of Promises in a JS context is the ability to 'branch' the 'then' functions.

One promise can provide data to multiple other promises that do independent things with it.

In Go, take this example code:

		p1 := promise.New(func(resolve func(interface{}), reject func(error)) {
			time.Sleep(time.Millisecond * 1000)
      
			fmt.Println("1 resolved")
			resolve(uint8(1))
		})

		p1doubled := p1.Then(func(data interface{}) interface{} {
			num := data.(uint8)

			time.Sleep(time.Millisecond * 2000)
			fmt.Println("double resolved")

			return num * 2
		})

		p1tripled := p1.Then(func(data interface{}) interface{} {
			num := data.(uint8)

			time.Sleep(time.Millisecond * 500)
			fmt.Println("triple resolved")

			return num * 3
		})

		oneD, _ := p1.Await()
		one := oneD.(uint8)

		twoD, _ := p1doubled.Await()
		two := twoD.(uint8)

		threeD, _ := p1tripled.Await()
		three := threeD.(uint8)

		fmt.Println(one)
		fmt.Println(two)
		fmt.Println(three)

The expected behaviour would be for it to print:

1 resolved
triple resolved (500ms later)
double resolved (1500ms after that)
1 (all at once)
2 (^)
3 (^)

However the current behaviour assumes all promises exist as 'chains' with no branches. This also causes 'waterfall' behaviour with its execution.

1 resolved
double resolved (2000ms later)
triple resolved (500ms after that)
6 (all at once)
6 (^)
6 (^)

I assume the former would actually be the intended behaviour for the library given the comments for the Then function:

	// Appends fulfillment to the promise,
	// and returns a new promise.

The Then function distinctly does not return a new promise, it returns the same promise, adding the fulfilment function to the list.

The fix is relatively simple, copy the Promise every time the Then function is called to branch it off, actually returning a new Promise.

The Catch function should not return a new promise, but the old promise, it should not copy it.

Essentially this:

// Then appends a fulfillment handler to the Promise, and returns a new promise.
func (promise *Promise) Then(fulfillment func(data interface{}) interface{}) *Promise {
	promise.mutex.Lock()
	defer promise.mutex.Unlock()

	promiseCopy := promise.copy()

	switch promiseCopy.state {
	case pending:
		promiseCopy.wg.Add(1)
		promiseCopy.then = append(promiseCopy.then, fulfillment)
	case fulfilled:
		promiseCopy.result = fulfillment(promiseCopy.result)
	}

	return promiseCopy
}

// copy creates a copy of a Promise that resolves when it does, it returns the new Promise.
func (promise *Promise) copy() *Promise {
	p := New(func(resolve func(interface{}), reject func(error)) {
		data, err := promise.Await()

		if err != nil {
			reject(err)
			return
		}

		resolve(data)
	})

	return p
}

This obviously changes the behaviour of the library in a breaking manner.

Would a PR like this be accepted? I would assume a major version bump to v2 would have to be done?

Alternatively I can just create a fork with the different behaviour.

What are your thoughts?

Thanks.

@chebyrash
Copy link
Owner

chebyrash commented Apr 14, 2020

Hi @Mike-Dax, very interesting point indeed!

image
I did a quick test to confirm this behaviour in JS

I agree that we should follow the same behaviour as JS since it's an actual feature that can be used by users.

In terms of breaking and versioning - initially I planned v2 as a release where we finally adopt Go's Generics which, in my opinion, is more of a breaking change than correcting existing behaviour.

Let me know if you want to go ahead and implement a fix for this.

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 a pull request may close this issue.

2 participants