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

Refactoring: Remove LegacyPromise #4430

Closed
bthorben opened this issue Mar 10, 2014 · 1 comment · Fixed by #4729
Closed

Refactoring: Remove LegacyPromise #4430

bthorben opened this issue Mar 10, 2014 · 1 comment · Fixed by #4729
Labels

Comments

@bthorben
Copy link
Contributor

All LegacyPromises should be refactored into native promises.

Optional: As this refactoring requires some thought anyway, it's probably a good idea to also make sure that all uses promises are actually needed. An example of doubtful usage would be in worker.js on line 276 inside wphSetupGetPage

@yurydelendik
Copy link
Contributor

#4700 creates PDFJS.createPromiseCapability(), which returns object with promise, resolve, and reject.

Let's try these rules/patterns, if this will not work, let's change that:

  1. async functions shall return promise (never capability or resolve/reject methods):

    function example() {
      return new Promise(function (resolve) {
        var request = requestSomething();
        request.onload = function (x) { resolve(x * 5); });
      });
    }
    

    or

    function example() {
       return Promise.resolve(5 * 5);
    }
    
  2. functions shall create PromiseCapability only with goals to store it as a private object field or pass to the function:

    function ExampleObject() {
      this._capability = createPromiseCapability();
    }
    ExampleObject.prototype = {
      get promise() {
        return this._capability.promise;
      }
    }
    

    or

    function example() {
      var capability = createPromiseCapability();
      requestSomething(capability);
      return capability.promise;
    }
    

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants