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

Possible issue with subclassing #11

Closed
Bnaya opened this issue May 17, 2016 · 2 comments
Closed

Possible issue with subclassing #11

Bnaya opened this issue May 17, 2016 · 2 comments

Comments

@Bnaya
Copy link

Bnaya commented May 17, 2016

https://github.com/poelstra/ts-promise/blob/master/src/lib/Promise.ts#L371-L375

var slave = new (Object.getPrototypeOf(this).constructor)(internalResolver);

I'm extending promise and using different constructor signature

import {Promise} from 'ts-promise';

export default class ImageFromUrl extends Promise<HTMLImageElement> {

    protected imageElement: HTMLImageElement;
    static ERROR_IMAGE_LOAD_FAIL: number = 1;

    constructor(url: string, {crossOriginAnonymous}: { crossOriginAnonymous: boolean } = { crossOriginAnonymous: true }) {
        super();

        this.loadHandler = this.loadHandler.bind(this);
        this.errorHandler = this.errorHandler.bind(this);

        this.imageElement = document.createElement('img');

        if (crossOriginAnonymous) {
            this.imageElement.crossOrigin = 'Anonymous';
        }

        this.imageElement.addEventListener('load', this.loadHandler);
        this.imageElement.addEventListener('error', this.errorHandler);

        this.imageElement.src = url;
    }

    protected loadHandler() {
        this.cleanup();
        this.resolve(this.imageElement);
    }

    protected errorHandler() {
        this.cleanup();
        this.reject(ImageFromUrl.ERROR_IMAGE_LOAD_FAIL);
    }

    protected cleanup() {
        this.imageElement.removeEventListener('load', this.loadHandler);
        this.imageElement.removeEventListener('error', this.errorHandler);
    }
}

when using the then method, the library is trying to create new instance of my subclass, with a function as the parameter but then it tried to load a new image again.
I think the library should use plain Promise when chaining

@poelstra
Copy link
Owner

Interesting question. It seems what you're trying to achieve is going to be tricky, because of the way Promises are spec'ed.

In your use-case, returning the plain Promise from .then() would indeed make sense, but it would break the use-case of someone extending Promise to e.g. have some nice extra helpers like .map(), and expects these helpers to still be available on the result of .then().

The Promise spec seems to target the latter use-case, and that's what I've also basically taken over.
See e.g. domenic/promises-unwrapping#87 (comment) for an in-depth discussion on this. Basically, the Promise spec says that the subclassed constructor needs to accept similar arguments as the base class.

FWIW, what I did in the past in a similar situation (http request wrapper), was to not subclass from Promise, but instead put e.g. a .result() method on the class that returns that Promise.
Also, in this case, if you don't expect your ImageFromUrl to be subclassed itself, you could consider transforming into a 'normal' function instead, which could then return a 'plain' Promise.

I'm going to close this issue for now, as I don't see how I could keep the spirit of the spec, and still allow 'your' use-case. Feel free to discuss this further, though, if you have a good idea to do this.

@Bnaya
Copy link
Author

Bnaya commented May 17, 2016

Thanks for your reply
I was needing a quick fix so i've made a fork.
I need the subclass for other reasons
My solution might be flawed but i need to handle it as it :)

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