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

change "if x is a promise" to something non-circular and clear #240

Open
donhatch opened this issue Oct 27, 2016 · 7 comments
Open

change "if x is a promise" to something non-circular and clear #240

donhatch opened this issue Oct 27, 2016 · 7 comments

Comments

@donhatch
Copy link

The promises/A+ spec currently has a clause saying "If x is a promise, adopt its state [3.4]:"
where footnote [3.4] is: "Generally, it will only be known that x is a true promise if it comes from the current implementation. This clause allows the use of implementation-specific means to adopt the state of known-conformant promises."

Notice that, according to the earlier definition of "promise", "If x is a promise" means "If x is an object or function with a 'then' method whose behavior conforms to this specification", where "this specification" includes the clause in question.

In other words, the definition of "promise" is circular.

If [3.4] is an attempt to clarify, it fails.

The clause in question should be changed to something non-circular and clear:

  • Do not use the words "if x is a promise" or any other self-referential phrase here.
  • If the clause is to say "if SOMETHING(x), adopt its state", then clearly say what is meant by SOMETHING(x).
    In other words, if it's intended that the implementation has latitude here, then clearly say how much latitude,
    so that it will be meaningful to ask, and possible to tell, whether any particular straightforward implementation of SOMETHING(x)
    is conformant or not from looking at the source code.

In particular, it should be possible to discern whether each of the following possible
implementations of the boolean function SOMETHING(x) is conformant:

  • function(x) { return typeof(x.then) == 'function'; }
  • function(x) { return typeof(x.then) === 'function'; }
  • function(x) { return x instanceof SomePromisesImplementation.Promise || x instanceof SomeOtherPromisesImplementation.WhateverItCallsAPromise; }
  • function(x) { return false; }
  • function(x) { return true; }
  • function(x) { return Math.random() >= 0.5; }
@donhatch
Copy link
Author

I think one reasonable possible resolution would be to change "If x is a promise, adopt its state [3.4]:"
to "(Optional) If x is known to be a promise, adopt its state [3.4]:".

Even though this is still self-referential, I believe this will make it clear and meaningful.

In particular, the questions of whether the 6 SOMETHING(x) functions shown at the end of my original post are conformant can then be answered: NO, NO, YES (assuming we trust the two implementations are conformant to the entire spec), YES, NO, NO.

@ForbesLindesay
Copy link
Member

x instanceof Promise is the kind of check that 3.4 is suggesting. "whose behavior conforms to this specification" is the key part of the "promise" definition here. The only way to know than x conforms to this specification is if you verify that it is a known promise implementation. If you know which promise implementation is being used, it's ok to use a more optimised approach to adopt the state of that promise.

If you feel this can be made more clear, perhaps you can submit a pull request?

@donhatch
Copy link
Author

Thanks @ForbesLindesay . From what you're saying, it looks like my interpretation of this clause is correct.

I'll put together a pull request with the wording I suggested in my previous comment, maybe also adding some of your words to footnote 3.4.

donhatch added a commit to donhatch/promises-spec that referenced this issue Oct 27, 2016
Also added some more words of clarification of intent to footnote 3.4,
loosely based on discussion with ForbesLindesay.
Closes promises-aplus#240.
donhatch added a commit to donhatch/promises-spec that referenced this issue Oct 27, 2016
Mention `x instanceof Promise` as an example of the kind of check
being suggested.  This is a further improvement for promises-aplus#240.
donhatch added a commit to donhatch/promises-spec that referenced this issue Oct 28, 2016
More polishing for promises-aplus#240: implementations SHOULD (not MAY) optimize the case
when x is known to be a promise, per discussion with @bergus.
@donhatch
Copy link
Author

I made the pull request (#241) and revised it a few times based on review discussion. I'm now very happy with it; I think it completely resolves the problem.

For reference, the proposed new wording is:

(Recommended) If `x` is known to be a promise, adopt its state [3.4]:
...
[3.4] Generally, it will only be known that `x` is a true promise if it comes from the
current implementation.  This clause allows the use of implementation-specific means to
adopt the state of known-conformant promises, which may be identified by a test such as
`x instanceof Promise`. This allows optimisations by not requiring the more general
thenable-handling procedure with its repeated value inspection.

It looks to me like @ForbesLindesay, @bergus, and @briancavalier are on board with the above proposed new wording.
But @domenic isn't; in his Nov 1 comment:

In general I am -1 on this change. I don't think the current spec is ambiguous and people haven't had any issues implementing it many times.

So we're back to the question of whether to make any change here at all.
I'll try to make one more appeal for it here. Here goes.


@domenic, I get that you think this part of the spec is good enough and not worth putting much more thought into.

The others (@ForbesLindesay, @bergus, @briancavalier) have put thought into it, and they've arrived at this tighter version of this section that I think is really good and completely resolves the issue.

Unless you actually think this change makes it worse overall, would you be willing to let the change through?

Here's why I think it's important.

The world is learning how promises work, in large part through discussions that refer to the promises/A+ spec, since it's the primary and best reference. In fact, it's the only reference that has the level of precision and completeness that's appropriate and useful for most of these discussions. (In contrast, the ES6 promises spec, for example, in addition to being implementation-specific, uses language that is too complicated and convoluted for it to be useful in resolving most of the questions I've seen.)

In particular, the wording of this section becomes significant when I discuss your spec with people, e.g. when asking or answering questions on stackoverflow, or if I write an implementation, or when I review someone else's implementation.
Without the proposed tightening, I find myself saying things of the form:

According to the promises/A+ spec: [old wording],
which obviously can't be taken literally since that wouldn't make sense,
but I've talked to the spec authors about it
and I can tell you with confidence that they did have something
coherent in mind here, and what they really meant was: [new wording].

which is kind of wordy and embarrassing.

Acknowledged, people have managed to implement it many times in spite of this hurdle, and maybe all of them even inferred your intent correctly-- I haven't checked.

In any case, after this change, I'll be able to say the following instead, which I very much prefer:

According to the promises/A+ spec: [new wording].

That's a significant reduction in noise and distraction which will increase the quality of the global discussion, and it will also reflect better on you and on the great work you've put into this project.

What do you think? Will you let it through?

@donhatch
Copy link
Author

@ForbesLindesay, what is the status of this?

My reading of what has happened so far is

Does this depend on domenic? @ForbesLindesay, would you be able to accept the pull request?

@domenic
Copy link
Member

domenic commented Dec 22, 2019

I am paying attention and my objection stands.

@donhatch
Copy link
Author

Hi Domenic,

Great that you're paying attention.

Please give an argument supporting your "I don't think the spec is ambiguous" that addresses the issue and the replies I've given you so far. Your offhand dismissal without explanation is unhelpful and comes off as disrespectful of the thought and work that I and the others have put into describing and attempting to address the issue.

Your "people haven't had any issues implementing it many times." is specious, and (even if reworded without the hyperbole), I suspect, false. I ask you to either start engaging thoughtfully and respectfully, or get out of the way.

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