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 Web IDL language support #1102

Closed
wants to merge 1 commit into from
Closed

Add Web IDL language support #1102

wants to merge 1 commit into from

Conversation

grorg
Copy link

@grorg grorg commented Feb 12, 2017

Hi. This is my first attempt at adding a language. I'm not sure I got everything right - the examples page has this language greyed out, for example. It seems that it tries to load content from github?

I'm happy to change anything necessary.

WebIDL features not yet supported: extended attributes and predefined error/exception names.

pattern: /(class\s+)[a-z0-9_]+/i,
lookbehind: true
}
});
Copy link
Member

@LeaVerou LeaVerou Feb 13, 2017

Choose a reason for hiding this comment

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

What's the point of calling insertBefore for webidl right after you've defined webidl? insertBefore is used to modify existing languages, if you're defining a language yourself you don't need it, just put "class-name" in the object literal.

Also, your dependencies list C-like, but here you seem to be extending C. These are two different language definitions.

Copy link
Author

Choose a reason for hiding this comment

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

I copied the cpp implementation, so maybe there is something in the insertBefore code that it needed. Either way, I don't understand so I'm happy to do whatever.

As for the dependencies, you're right that it's not clear. WebIDL isn't technically C, C-like, or CPP. I figured it would be ok to extend from cpp though, since it does have fairly similar syntax.

Copy link
Member

@LeaVerou LeaVerou Feb 16, 2017

Choose a reason for hiding this comment

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

Just add the "class-name" token before the keyword token. There's no need for it to be separate. What the cpp language is doing is weird too, @zeitgeist87 what was the rationale?

As for the dependencies, I don't mind which one you inherit from, just be consistent between your code and the components declaration, otherwise people will get a download that doesn't work. Perhaps look over their examples to decide which of the three is closer to WebIDL?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@LeaVerou The c language actually deletes the class-name property from clike, so if you inherit from c and you want to put class-name at a certain position, you have to use insertBefore(). However if you inherit from clike, you can directly add class-name to the language.

Copy link
Member

@LeaVerou LeaVerou Feb 16, 2017

Choose a reason for hiding this comment

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

I see. So the main blocker here is that @grorg needs to decide whether he's extending C, C++ or C-like. I think the latter is probably a better fit, but he knows WebIDL better.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the tips @LeaVerou and @zeitgeist87. I'll update the patch soon.

he knows WebIDL better

Hahahahaha. If only.

@LeaVerou
Copy link
Member

I've added a few comments, though I'd wait for some more review from the other maintainers before merging.

But I'm really excited about this! I assume you're using Prism in specs then?

@grorg
Copy link
Author

grorg commented Feb 14, 2017

But I'm really excited about this! I assume you're using Prism in specs then?

I was using it in some WebGPU stuff. Neither prism or highlight.js supported WebIDL. I decided prism was easier :)

@mAAdhaTTah
Copy link
Member

@grorg Are you able to make the changes suggested here? We'd love to land this!

@grorg
Copy link
Author

grorg commented Jun 6, 2018

Yeah, thanks for poking me. I'll clean it up and post again soon.

@RunDevelopment
Copy link
Member

Closed in favor of #3107.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants