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 clearer. #241

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

donhatch
Copy link

Also added some more words of clarification of intent to footnote 3.4,
loosely based on discussion with @ForbesLindesay .
Closes #240.

Also added some more words of clarification of intent to footnote 3.4,
loosely based on discussion with ForbesLindesay.
Closes promises-aplus#240.
Mention `x instanceof Promise` as an example of the kind of check
being suggested.  This is a further improvement for promises-aplus#240.
@donhatch
Copy link
Author

I added the second commit as an afterthought.
This pull request would be simpler and probably better as a single commit,
but I'm not sure of the best way to make it so at this point, or whether it matters.
Any advice?

@bergus
Copy link

bergus commented Oct 27, 2016

This step should not be marked "optional". The idea behind adopting the state is a) not checking whether the result value is a thenable again and again, and based on that b) allow optimisations that fix memory problems, like in the case of deeply recursive promise chaining.

@donhatch
Copy link
Author

@bergus If you are saying this should be required rather than optional, then it needs to say exactly what is required. "If x is known to be a promise" isn't enough, since that doesn't make it clear how hard the implementation must try before it decides "I know x is a promise" or "I don't know x is a promise".

In particular, if the spec were to just say "If x is known to be a promise" here, an implementation can conform by simply not checking anything here-- if it doesn't check, then it doesn't know x is a promise, right? So that would make this part effectively Optional anyway, but without coming out and saying so, which makes me uncomfortable as a reader since it makes me think (correctly) that I may be misunderstanding the author's intent.

If you're saying literally that the implementation MUST test for "x instanceof CurrentImplementation.Promise", that needs to be stated (more strongly than just as a suggestion of one possibility mentioned in the footnote, which is where I put it in my proposed patch so far).

What would you like it to say?

@bergus
Copy link

bergus commented Oct 28, 2016

an implementation can conform by simply not checking anything here

Yes, that's true - especially if the implementation would be so foolish as to only call .then(resolve, reject) anyway as its means to "adopt the state", so there's no need to distinguish promises from other thenables. (For example, ES6 does just that).

However, the intent is that an implementation should use a more efficient adoption strategy, so I think it's fine as it stands. If you insist, you could add a "SHOULD".

@donhatch
Copy link
Author

Yeah, I think it should have a SHOULD there if that's the intent (what I have written so far is effectively a MAY).
Pondering how to word it.

@donhatch
Copy link
Author

Oh, I see how to do it-- I can just change "(Optional)" to "(Recommended)", since RECOMMENDED is the same as SHOULD.

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.
@briancavalier
Copy link
Member

👍 Thanks @donhatch. Strengthening "Optional" to "Recommended" seems to capture what I think we intended in the first place.

Any additional concerns, @domenic, @bergus, @ForbesLindesay?

Copy link
Member

@ForbesLindesay ForbesLindesay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure about this. If the value is a promise, you MUST adopt its state. The key is that you MAY deviate from the exact algorithm in this document to adopt that state. Perhaps updating the note is enough. Alternatively we could inline some of the description like:

If x is a promise, adopt its state. You may do this via the method described in 2.3.3.

@@ -108,7 +108,7 @@ If a promise is resolved with a thenable that participates in a circular thenabl

1. Implementations may allow `promise2 === promise1`, provided the implementation meets all requirements. Each implementation should document whether it can produce `promise2 === promise1` and under what conditions.

1. 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.
1. 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`, as an optimization to avoid the more general thenable-handling procedure described in subsequent sections.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence runs on a bit. Maybe we could have:

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 is to allow optimization, which may require avoiding the more general thenable-handling procedure described in subsequent sections.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ForbesLindesay "may require avoiding" sounds odd. How about

This does allow optimisations by not requiring the more general thenable-handling procedure with its repeated value inspection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like that much more. What about:

This allows optimisations by not requiring the more general thenable-handling procedure with its repeated value inspection.

@donhatch
Copy link
Author

I rephrased footnote 3.4 as agreed on by @ForbesLindesay and @bergus. It all looks very clear to me now.

However, I no longer see the comments in which they suggested this latest wording... ?? Those comments started with something like "this sentence runs on a bit", if I remember correctly.

@donhatch
Copy link
Author

Oh I see, I received those comments via email; for some reason they aren't showing up here on #241 like I expected they would.

@donhatch
Copy link
Author

I see the comments in question now. They are inside an initially collapsed "Show outdated" bellows.

@domenic
Copy link
Member

domenic commented Nov 1, 2016

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.

@bergus
Copy link

bergus commented Nov 1, 2016

@domenic Well at least ES6 promises did fail to implement this particular step, so maybe adding the "(Recommended)" wouldn't go amiss :-)

@donhatch
Copy link
Author

donhatch commented Nov 2, 2016

Hey @domenic, thanks for taking a look.

I'm certainly disappointed that you and I see such different things when we look
at that section of the spec, especially after I thought we were on the verge of making
what I perceive to be a quite good and important fix.

If you haven't found anything that's been said so far to be compelling,
I doubt anything I could say would convince you that there's a problem,
so I won't try to argue it further.

For the record, my feeling on the relative importances of the pieces of this proposed change is:

  1. Adding "(Recommended)" and "known to be" to the main text is essential--
    without those, the stated requirement is impossible to satisfy, and doesn't express your intent.
  2. Including the example criterion x instanceof Promise in the footnote is secondary--
    it definitely adds value, but I wouldn't say the spec is broken without it.
  3. I'm not attached to anything else about the content or rephrasing of the footnote,
    though I do like what we've arrived at in the proposed patch at this moment-- it's very clear to me now.

@bergus's latest suggestion of only adding "(Recommended)" would address so little of the problem,
as I see it, that I would not want to have my name on that commit.

So, I'll retract this pull request, and I'll close #240 citing your #241 (comment) above, unless you want to do that.
Let me know; I'll close it in a day or two if I don't hear otherwise from you.

@donhatch
Copy link
Author

After thinking more about this, I'm going to give it one more pitch.

Since the proposed patch contents seem to be stable, and the discussion is back to the higher-level question of whether to do this at all, I'm going to take it out of this trench and back up to the issue thread #240. See you there.

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 this pull request may close these issues.

5 participants