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

Custom Elements: CustomElementRegistry #299

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

prasannavl
Copy link

@prasannavl prasannavl commented Nov 7, 2018

[WIP] Initial impl of CustomElementRegistry

I do have a few questions before completing this:

  1. How exactly do I represent a Constructor here? (51e683c#diff-55e0c9ca42d6126fc52bcae5a9a2bf95R38)
  2. Is it okay to unwrap, this? Initially I did a no_return, but then added the error type, and as such the Result<(), Err>. Does this fit the project conventions? (51e683c#diff-55e0c9ca42d6126fc52bcae5a9a2bf95R41)
  3. Again, constructor type. But also, can Option be returned. Does this fit the project conventions, or should this be a result with empty error/not found error? What's the best way to represent undefined here. (51e683c#diff-55e0c9ca42d6126fc52bcae5a9a2bf95R48)
  4. I followed this convention by looking at other implementations, but it is correct to unwrap this? (51e683c#diff-55e0c9ca42d6126fc52bcae5a9a2bf95R59)

[WIP] Initial impl of `CustomElementRegistry`
Copy link
Owner

@koute koute left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Initial review done. Two general comments:

  1. You forgot to add a mod custom_elements; to src/webapi/mod.rs, so this is currently dead code.
  2. In the final PR I'd like to see some tests. (:

/// and querying registered elements. To get an instance of it, use the window.customElements property.
///
/// [(JavaScript docs)](hhttps://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry)
/// https://html.spec.whatwg.org/multipage/custom-elements.html#customelementregistry
Copy link
Owner

Choose a reason for hiding this comment

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

Links to the specs should be in a normal comment, that is // instead of ///. (:


error_enum_boilerplate! {
/// A enum of the exceptions that CustomElementRegistry.define() may throw
DefineError,
Copy link
Owner

Choose a reason for hiding this comment

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

A little more verbose name would be better here, I think. Maybe CustomElementDefineError? It's a little long, but simply DefineError seems to generic.

///
/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/define)
/// https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-define
pub fn define( &self, name: &str, constructor: Any) -> Result<(), DefineError> {
Copy link
Owner

@koute koute Nov 8, 2018

Choose a reason for hiding this comment

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

Hmmm.... we currently don't have a type which can represent a constructor. According to this a constructor can be detected with some tricks so I guess we technically could add one?

You'd define such a type using the usual #[derive(ReferenceType)], however you would omit the #[reference(instance_of = "...")] line and instead implement the InstanceOf trait yourself, and put the detection code inside of it. (You can take a look at e.g. the definition of the error_boilerplate! for an example on how to impl the InstanceOf) Then once we have this type you could use it here and in the return type of get.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the check for constructors is more involved, because custom elements require actual ES6 classes (and they must have the appropriate extends).

I think this also ties into the ability to define JS classes in stdweb, and we don't currently have a thorough plan for that.

I suppose we could create a simple struct Constructor(Reference) type for now (without any runtime checking).

Copy link
Owner

Choose a reason for hiding this comment

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

I think the check for constructors is more involved, because custom elements require actual ES6 classes (and they must have the appropriate extends).

Isn't the ES6 class syntax mostly a syntax sugar? I grabbed one of the examples off MDN of custom elements and I've modified the element definition to use the old-style of defining classes, and it looks like the example still works...? (At least in Firefox.) I wonder if that's just an accident? (Granted, I'm getting an InvalidStateError exception from somewhere once the control returns to the main loop, but the example still works, and my manual class-defining-voodoo might be slightly not what it expects, so, uh, I'm confused.)

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I haven't had the time to get back on this yet.

But, AFAIK, ES6 is not just sugar though. It can be done without ES6 syntax - using Object.setPrototypeOf, or Reflect.setPrototypeOf to be more compliant - however if I'm correct, these are reflection API in the JS engine, and are less performant than es6 syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

@koute No, it's not just sugar: the ability to extend host built-ins (like Array, Error, HTMLElement, etc.) doesn't work with ES5 classes, but it does with ES6 classes (which is why transpilers like Babel cannot extend those classes, since Babel compiles ES6 classes into ES5 classes).

As @prasannavl said, there are some ways to workaround it with ES5 classes, but it's not simple or easy (which is why almost nobody does it).

Could you post your code? When I tried using ES5 classes in Chrome it gave me errors when I called customElements.define.

Copy link
Owner

Choose a reason for hiding this comment

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

Could you post your code? When I tried using ES5 classes in Chrome it gave me errors when I called customElements.define.

I don't have in on hand anymore, however it really was just one of the examples picked at random with the class definition replaced with your garden variety old-style prototype-based JS class definition.

So this begs the question - is it actually possible to detect that something is a genuine ES6 class?

/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/define)
/// https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-define
pub fn define( &self, name: &str, constructor: Any) -> Result<(), DefineError> {
js!(
Copy link
Owner

Choose a reason for hiding this comment

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

You need to use js_try! here.

/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/whenDefined)
/// https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-whendefined
#[cfg(feature = "experimental_features_which_may_break_on_minor_version_bumps")]
pub fn whenDefined( &self, name: &str ) -> TypedPromise<(), SyntaxError > {
Copy link
Owner

Choose a reason for hiding this comment

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

This should be named in snake case, that is when_defined

/// https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-whendefined
#[cfg(feature = "experimental_features_which_may_break_on_minor_version_bumps")]
pub fn whenDefined( &self, name: &str ) -> TypedPromise<(), SyntaxError > {
let promise: Promise = js!( return @{self}.whenDefined(name); ).try_into().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this unwrap is fine. (As long as we have a guarantee that whenDefined will always return a Promise.)

@@ -0,0 +1,62 @@
use webcore::value::Reference;
webapi::error::TypeError;
Copy link
Owner

Choose a reason for hiding this comment

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

You're missing use here. (:

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.

None yet

3 participants