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

Add the symbol type #377

Merged
merged 1 commit into from
Jul 31, 2017
Merged

Add the symbol type #377

merged 1 commit into from
Jul 31, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented Jun 27, 2017

Fixes part of https://www.w3.org/Bugs/Public/show_bug.cgi?id=27553. Fixes #301.

This will be necessary for whatwg/dom#469, as feedback in whatwg/dom#208 has been that we should restrict the type of the group to either a string or a symbol.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=27553 also contains speculative stuff about interfaces declaring their own symbols which is not included here.


Preview | Diff

@annevk
Copy link
Member

annevk commented Jun 27, 2017

Should static Symbol objects also be exposed on instances or should we fix/change that behavior for Symbol objects?

@domenic
Copy link
Member Author

domenic commented Jun 27, 2017

Oh, hmm, I forgot people could start declaring constants of a type the moment we added the type. Interesting. I guess this solves more of https://www.w3.org/Bugs/Public/show_bug.cgi?id=27553 than I thought.

Yes, let me add that.

@domenic
Copy link
Member Author

domenic commented Jun 27, 2017

Wait, no, you cannot declare constant symbols yet. Since there is no way of representing a constant symbol value in Web IDL. So we are safe for now, and can expand to add that functionality later when needed.

@annevk
Copy link
Member

annevk commented Jun 28, 2017

Oh right, https://www.w3.org/Bugs/Public/show_bug.cgi?id=27553#c4 declares a static and that won't have that issue. Okay.

@tobie tobie self-requested a review July 31, 2017 09:46
Copy link
Collaborator

@tobie tobie left a comment

Choose a reason for hiding this comment

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

This LGTM.

It does allow editors to add

interface Foo {
  static Symbol bar;
};

I imagine the purpose of Foo.bar would be limited to similar use cases as Symbol.iterator (i.e. a hook to get hold of a symbol), no?

If so, should we add some constraints as to how this can be used in practice? E.g. add to the static attribute section something along the lines of: Static attributes of type {{Symbol}} must be [=readonly=] and their value must always reference the same symbol.

@domenic
Copy link
Member Author

domenic commented Jul 31, 2017

It does allow editors to add

No it doesn't. It would have to be an attribute or readonly attribute. Which is indeed unusual, but I don't think it's something we should have special rules for.

If we want to allow Foo.bar for similar purposes as Symbol.iterator, we'll need a lot more work, as we won't want those to be getters---we'll want them to be data properties.

@tobie
Copy link
Collaborator

tobie commented Jul 31, 2017

No it doesn't. It would have to be an attribute or readonly attribute. Which is indeed unusual, but I don't think it's something we should have special rules for.

Yeah, that's what I meant. Sorry for the sloppy WebIDL snippet.

If we want to allow Foo.bar for similar purposes as Symbol.iterator, we'll need a lot more work, as we won't want those to be getters---we'll want them to be data properties.

Oh… I hadn't thought about this.

So I guess we're good, then?

@domenic
Copy link
Member Author

domenic commented Jul 31, 2017

Probably, although I'm unsure if we should merge this until we have implementer support on whatwg/dom#469...

@tobie
Copy link
Collaborator

tobie commented Jul 31, 2017

Well, it does solve #301 which needs fixing anyway and doesn't have a simpler fix afaik, no? If that's right, then we should go ahead and merge.

@domenic
Copy link
Member Author

domenic commented Jul 31, 2017

Good point, yeah.

@domenic domenic merged commit d6c3ddb into master Jul 31, 2017
@domenic domenic deleted the symbols branch July 31, 2017 15:38
@bzbarsky bzbarsky mentioned this pull request Feb 23, 2018
@bzbarsky
Copy link
Collaborator

bzbarsky commented Mar 2, 2018

@domenic So I'm a little confused about this change. The grammar allows writing something like this:

void foo((symbol or long) arg);

but the processing model for unions (and overload resolution) seems to not know about symbols? I thought the idea was to only allow them to be passed via any, but it looks like syntax got added for them?

@domenic
Copy link
Member Author

domenic commented Mar 7, 2018

Yeah, it looks like an oversight.

The idea was to add actual syntax for them in support of whatwg/dom#469. But, that PR is stalled on implementer interest, so this hasn't seen any real use in the wild. As you can see from the above, we mostly merged it because it fixed #301.

Do you think we should fix up the oversights and add union and overload processing for symbols, or walk things back and remove the parts of this patch that aren't necessary for solving #301?

@bzbarsky
Copy link
Collaborator

bzbarsky commented Mar 7, 2018

No strong opinions either way.

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.

None yet

4 participants