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

Issue #5552 - Update whenDefined steps to resolve with the class #5833

Merged
merged 3 commits into from
Sep 8, 2020

Conversation

WebReflection
Copy link
Contributor

@WebReflection WebReflection commented Aug 17, 2020

@ExE-Boss
Copy link

I’m pretty sure that “implementers” refers to browser engines and other implementations of the HTML specification.

@annevk
Copy link
Member

annevk commented Aug 17, 2020

This seems equivalent to #5831.

Equivalent to get(name), you want to resolve with the entry's constructor (and xref it).

Copy link

@ExE-Boss ExE-Boss left a comment

Choose a reason for hiding this comment

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

source Outdated Show resolved Hide resolved
@WebReflection WebReflection force-pushed the issue-5552 branch 3 times, most recently from 5efeb24 to 792fc72 Compare August 17, 2020 11:07
@WebReflection
Copy link
Contributor Author

This seems equivalent to #5831.

Except it has all bugs attached ... maybe we can flag that one as duplicated, or move all filed bugs in there? Filed bugs also point at this URL ... didn't notice there was already a PR

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I also noticed that whenDefined() doesn't seem to return promise at the moment, but perhaps that's best tackled separately.

source Outdated Show resolved Hide resolved
@WebReflection WebReflection force-pushed the issue-5552 branch 2 times, most recently from 7204a6e to bd36869 Compare August 17, 2020 13:59
@rniwa
Copy link

rniwa commented Aug 21, 2020

WebKit patch is ready to go: https://bugs.webkit.org/show_bug.cgi?id=215562

@annevk
Copy link
Member

annevk commented Aug 21, 2020

@mfreed7 @smaug---- @EdgarChen @domenic thoughts? Seems reasonable to me.

@WebReflection you'll have to sign https://participate.whatwg.org/agreement.

@WebReflection
Copy link
Contributor Author

@WebReflection you'll have to sign https://participate.whatwg.org/agreement.

Done 👍

@ExE-Boss
Copy link

ExE-Boss commented Aug 23, 2020

This should probably also have “Fixes #5552” in the PR description.


See GitHub Docs for details.

@EdgarChen
Copy link
Member

@mfreed7 @smaug---- @EdgarChen @domenic thoughts? Seems reasonable to me.

This seems reasonable to me.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Aug 24, 2020

@WebReflection given https://whatwg.org/ipr-policy#210-field-of-web-technologies I think we need your employer to sign the agreement as ad blocking does seem to be in that realm.

Other than that I think we're good to go here with agreement from two implementers (as well as a patch), tests, and a reviewed PR.

@WebReflection
Copy link
Contributor Author

@annevk I’m not contributing on behalf of my employer here, I’m even on vacation right now, and this is my own account, not the one used for my work.

Do I need to ask them? Looks like friction for no reason, and this contribution has nothing to do with ads.

Please let me know, thanks.

@annevk
Copy link
Member

annevk commented Aug 25, 2020

I checked and I think all is in order. I did notice that IDL wasn't updated and I pushed a commit for that. At this point I should let @domenic do the final review though.

@rniwa
Copy link

rniwa commented Aug 25, 2020

This PR has been implemented in WebKit as of https://trac.webkit.org/r266142.

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.

Should update the domintro too.

Marking as "Request changes" as @WebReflection definitely works in the field of web technologies, and as such needs their employer to sign the agreement.

@emilio
Copy link
Contributor

emilio commented Aug 26, 2020

I have a patch for Gecko in https://phabricator.services.mozilla.com/D88281 too fwiw, but I want to wait till WPT and the spec are updated.

@WebReflection
Copy link
Contributor Author

Just FYI, I've contacted my company about this but many, me included, are on vacation these days, so this might take a while.

Should update the domintro too.

@domenic anything I should do? Not sure I even know what that is, thanks.

@annevk
Copy link
Member

annevk commented Aug 28, 2020

@WebReflection basically

   <dt><var>window</var> . <code data-x="dom-window-customElements">customElements</code> . <code
   subdfn data-x="dom-CustomElementRegistry-whenDefined">whenDefined</code>(<var>name</var>)</dt>

   <dd>Returns a promise that will be fulfilled when a <span>custom element</span> becomes defined
   with the given name. (If such a <span>custom element</span> is already defined, the returned
   promise will be immediately fulfilled.) Returns a promise rejected with a
   <span>"<code>SyntaxError</code>"</span> <code>DOMException</code> if not given a <span>valid
   custom element name</span>.</dd>

needs updating to account for the fact that the promise has a specific fulfillment value. "domintro" are the green boxes you sometimes see in WHATWG standards that have a more developer-facing informal description of an API.

@WebReflection
Copy link
Contributor Author

basically ...

Done 👍

@WebReflection
Copy link
Contributor Author

Marking as "Request changes" as @WebReflection definitely works in the field of web technologies, and as such needs their employer to sign the agreement.

I've just submitted the agreement on behalf of the company I work for, I hope everything is fine now.

@annevk
Copy link
Member

annevk commented Sep 7, 2020

@WebReflection you'll need to make your membership of eyeo-gmbh public.

@annevk annevk requested a review from domenic September 7, 2020 08:36
@WebReflection
Copy link
Contributor Author

@annevk you mean the org, right? I'm on it 👍

@annevk
Copy link
Member

annevk commented Sep 7, 2020

@WebReflection
Copy link
Contributor Author

WebReflection commented Sep 7, 2020

@annevk I should be in the list now https://github.com/eyeo-gmbh ... anything else I should do/ask for? Thanks!

Edit I should have eyeo in the list of my orgs now

@annevk
Copy link
Member

annevk commented Sep 8, 2020

When I go to your profile I don't see it.

@annevk
Copy link
Member

annevk commented Sep 8, 2020

Also, it seems you undid my changes to this branch so the IDL is no longer correct.

@WebReflection
Copy link
Contributor Author

When I go to your profile I don't see it.

can you please try again? it had private visibility, but I've thought bots would see it regardless. Anyway, it's public now.

Also, it seems you undid my changes to this branch so the IDL is no longer correct.

which change? happy to put it back.

@WebReflection
Copy link
Contributor Author

@annevk I've rebased from upstream too, but I'm afraid I'm not sure what/where is the change you are mentioning. Any hint appreciated, thanks.

@ExE-Boss
Copy link

ExE-Boss commented Sep 8, 2020

@WebReflection
You need to change the return type of the WebIDL definition of CustomElementRegistry.whenDefined(…) from Promise<undefined> to Promise<CustomElementConstructor>.

@WebReflection
Copy link
Contributor Author

WebReflection commented Sep 8, 2020

@WebReflection
You need to change the return type of the WebIDL definition

done

source Show resolved Hide resolved
@annevk annevk dismissed domenic’s stale review September 8, 2020 14:43

It's been addressed.

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 8, 2020
@annevk annevk merged commit ed61fda into whatwg:master Sep 8, 2020
@annevk
Copy link
Member

annevk commented Sep 8, 2020

Thanks @WebReflection!

@WebReflection
Copy link
Contributor Author

Thanks @WebReflection!

no, thank you all for this, it looks tiny, but I believe it improves a lot usability 🎉

domenic pushed a commit to jsdom/jsdom that referenced this pull request Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

CustomElementsRegistry whenDefined then => Class
7 participants