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

Track the incumbent settings and active script in Promise callbacks #5722

Merged
merged 8 commits into from
Feb 3, 2021

Conversation

syg
Copy link
Contributor

@syg syg commented Jul 10, 2020

This depends on the host hooks that ecma262 is adding in tc39/ecma262#2086.

Open issues:

  1. This moves the step that captures the active script from the invocation of HostEnqueuePromiseJob to HostMakeJobCallback, i.e. from the point of scheduling the job to the earlier point of passing a callback to Promise.prototype.then. I think this is not observable but I'm not sure. For PromiseReaction Jobs, the active script or module is tracked by the built-in resolve and reject functions, so when they trigger the promise reaction and schedule the job, it'll propagate the same active script as at the time that the handler function was passed to Promise#then. For PromiseResolveThenable Jobs, the scheduling is synchronous with the call to resolve, so nothing changes here.

  2. How do I add the MDN sidebar icon? We need to say that the incumbent tracking in Promises is currently only supported in Firefox.


/index.html ( diff )
/infrastructure.html ( diff )
/webappapis.html ( diff )

@annevk annevk added do not merge yet Pull request must not be merged per rationale in comment impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation topic: script labels Jul 10, 2020
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Thanks so much; this looks like a great start.

I started doing some editorial review but stopped halfway. Probably we should settle the bigger questions first. However, the rearrangement I suggested might be nice to tackle earlier.

This moves the step that captures the active script from the invocation of HostEnqueuePromiseJob to HostMakeJobCallback, i.e. from the point of scheduling the job to the earlier point of passing a callback to Promise.prototype.then. I think this is not observable but I'm not sure.

I agree. Also I think this architecture makes more sense, and fits with how I've sketched out for Web IDL to behave in whatwg/webidl#902. I suspect the previous placement was just because we didn't have 262 hooks in the more-suitable place, so we captured the active script in HostEnqueuePromiseJob since that was where we had access to.

How do I add the MDN sidebar icon? We need to say that the incumbent tracking in Promises is currently only supported in Firefox.

For this you update https://github.com/mdn/browser-compat-data/ and then ensure that a corresponding MDN article references the browser-compat data. E.g. the MDN sidebar icon for https://html.spec.whatwg.org/#custom-handlers is added via the https://developer.mozilla.org/en-US/docs/Web/API/Navigator/registerProtocolHandler article which gets its data from https://github.com/mdn/browser-compat-data/blob/79bdc87d0c19c4ed9e6524c228c0ead81c44bea4/api/Navigator.json#L1793 .

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
@syg
Copy link
Contributor Author

syg commented Jul 11, 2020

Thanks for the MDN compat instructions. I'll tackle those next week.

@syg
Copy link
Contributor Author

syg commented Jul 13, 2020

I started looking at the MDN compat data but stopped since I don't know how we should organize this. Most compat data are obviously linked to a particular API or a tag or something, but this is compat for a particular behavior for certain APIs.

Should we be adding a new page? Should we be adding notes to the page for e.g. Promise.prototype.then() so that there's an asterisk that says incumbents are only tracked in Firefox?

My initial opinion is that since this is a cross-cutting behavior that affects multiple APIs that will be implemented in an all-or-nothing manner, it does deserve its own page instead of having notes on e.g. Promise.prototype.then().

@codehag Any opinions from Mozilla?

@codehag
Copy link

codehag commented Jul 14, 2020

cc'ing @chrisdavidmills as per our email discussion. We should be able to help. I can post the summary of actions once we settle on them.

@codehag
Copy link

codehag commented Dec 4, 2020

This took a while, sorry. MDN page is updated: https://wiki.developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#Incumbent_Realm

And patch to browser compat data is here: mdn/browser-compat-data#7568

@sideshowbarker
Copy link
Contributor

As noted in my review comments over at mdn/browser-compat-data#7568 (review), I suggest we consider using the title “Incumbent-realm tracking” to refer to this feature (in, e.g., browser-project platform-status pages, and in MDN and BCD).

@sideshowbarker
Copy link
Contributor

cc @whatwg/documentation

@codehag
Copy link

codehag commented Dec 8, 2020

Updated MDN title: https://wiki.developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#Incumbent_realm_tracking

And added your suggestions to the BCD pr

@syg
Copy link
Contributor Author

syg commented Dec 16, 2020

I'm still not sure how to use the BCD data that @codehag added to generate the side-box. After some fumbling I think the following is needed, but am not sure how to test if it works:

  1. Add a <dfn> so there's a place for BCD to link to. (Done in this PR.)
  2. Add "spec_url": "https://html.spec.whatwg.org/multipage/#incumbent-settings-object-tracking-in-promises" to the incumbent_settings_object_tracking entry in Promise.json. (Need to make a PR.)
  3. Wait for https://github.com/w3c/mdn-spec-links to regenerate, which should add an entry in html.json because the entries are keyed off the base URL according to SPECMAP.json
  4. Wattsi should then generate the MDN compat box.

@domenic @sideshowbarker Does that sound right?

Edit: With a hand-modified .cache/mdn-spec-links-html.json, I've confirmed that Wattsi generates the box as expected.

@sideshowbarker
Copy link
Contributor

I'm still not sure how to use the BCD data that @codehag added to generate the side-box. After some fumbling I think the following is needed, but am not sure how to test if it works:

  1. Add a <dfn> so there's a place for BCD to link to. (Done in this PR.)

  2. Add "spec_url": "https://html.spec.whatwg.org/multipage/#incumbent-settings-object-tracking-in-promises" to the incumbent_settings_object_tracking entry in Promise.json. (Need to make a PR.)

Yes

  1. Wait for https://github.com/w3c/mdn-spec-links to regenerate, which should add an entry in html.json because the entries are keyed off the base URL according to SPECMAP.json

Right. As far as waiting for mdn-spec-links to regenerate, that usually happens at least once a week, at the end of the week. But note that due to the MDN move to a completely new backend this week, it no longer provides the API endpoints I’ve so far been using to grab the data from MDN that I need for mdn-spec-links.

However, the new MDN system provides a new, different API that I can use to get the same data. So I just need to set aside a couple hours to update my MDN consumer code to use the new API. So ideally I can squeeze in some time to get that done this week, and there won’t be any noticeable delay in mdn-spec-links getting updated by the end of the week.

  1. Wattsi should then generate the MDN compat box.
    ...

Edit: With a hand-modified .cache/mdn-spec-links-html.json, I've confirmed that Wattsi generates the box as expected.

Yup. The Wattsi side of it should just work

@syg
Copy link
Contributor Author

syg commented Dec 16, 2020

@domenic @annevk Sounds like the next step is to get LGTM on the specific <dfn> wording before making the BCD PR. How do you feel about #incumbent-settings-object-tracking-in-promises?

@domenic
Copy link
Member

domenic commented Dec 16, 2020

I don't think we should have <dfn>s that aren't defining a term, but I think the whole infrastructure would work just fine with any id="incumbent-settings-object-tracking-in-promises" attribute, right? In which case I think putting such an id="" anywhere would be fine.

BTW I think this PR needs some rebasing from scratch, as sections seem to have moved around from underneath it... sorry about that.

@syg
Copy link
Contributor Author

syg commented Dec 16, 2020

Oh, does it work with any id? That sounds fine, I can just put an id. I'll double-check the rebase too.

syg added a commit to syg/browser-compat-data that referenced this pull request Dec 16, 2020
This is needed for Wattsi to generate an MDN compatibility box for the
HTML specification.

HTML PR: whatwg/html#5722
@syg
Copy link
Contributor Author

syg commented Dec 16, 2020

The rebase looks fine to me, organizationally.

@domenic
Copy link
Member

domenic commented Dec 16, 2020

Sorry about that, I was confused by all the stuff moving around. I now see that was intentional because we're grouping the job-related host hooks.

@sideshowbarker
Copy link
Contributor

Oh, does it work with any id? That sounds fine, I can just put an id.

Yeah, to be clear about the MDN-anno handling specifically, it doesn’t require the target ID to be for a dfn; in fact in practice, a lot of the existing spec IDs which MDN articles have links to are for headings.

sideshowbarker pushed a commit to mdn/browser-compat-data that referenced this pull request Dec 17, 2020
This is needed for Wattsi to generate an MDN compatibility box for the
HTML specification.

HTML PR: whatwg/html#5722
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM! I'll set a reminder to check back on the mdn-spec-links regeneration and merge this when that happens.

@syg syg changed the title (DO NOT MERGE) Track the incumbent settings and active script in Promise callbacks Track the incumbent settings and active script in Promise callbacks Dec 28, 2020
@syg syg removed the do not merge yet Pull request must not be merged per rationale in comment label Dec 28, 2020
Base automatically changed from master to main January 15, 2021 07:57
@domenic
Copy link
Member

domenic commented Jan 15, 2021

@sideshowbarker ping on https://github.com/w3c/mdn-spec-links, when you get a chance :)

@sideshowbarker
Copy link
Contributor

This should now be ready to merge, because the following are done:

image

(Sorry it took me so ridiculously long to get back to this; for the previous month and a half, I’ve been spending nearly all my time on https://github.com/mdn/content and https://github.com/mdn/browser-compat-data work.)

@domenic domenic merged commit 01574f3 into whatwg:main Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation topic: script
Development

Successfully merging this pull request may close these issues.

5 participants