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

WebIDL dependency #238

Closed
lanthaler opened this issue Apr 2, 2013 · 11 comments
Closed

WebIDL dependency #238

lanthaler opened this issue Apr 2, 2013 · 11 comments

Comments

@lanthaler
Copy link
Member

_From @sandhawke:_

So we're in the odd situation with JSON-LD-API of needing a normative reference to WebIDL, which might stay stuck in CR for some time. In this case, however, we can proceed to REC, even with it in CR, by following these instructions:

You'll need to provide evidence that the WebIDL used in your spec is stable, as follows:

To provide evidence that your WebIDL is stable and tested, you could use idlharness.js. example and run it against Firefox nightly and IE10. idlharness.js doesn't cover all of WebIDL. If you run into a limitation, contact Robin Berjon and he might be able to help.

I don't actually understand this at all. I'm hoping some other people do. If not, I'll work harder to figure it out.

-- Sandro

lanthaler added a commit that referenced this issue Apr 2, 2013
I had to modify the WebIDL slightly, see https://github.com/darobin/respec/issues/182.

This addresses #238.
@lanthaler
Copy link
Member Author

I've added (fa4d4ce) an idlharness test: http://json-ld.org/test-suite/idltest/

Unfortunately it fails with the following errors:

  • callback not yet supported
  • callback not yet supported
  • callback not yet supported
  • enum not yet supported

I've had a look at the idlharness repo. There was only a issue for dictionaries (w3c/testharness.js#18) so I've opened two new issues:

I've also discovered a bug (?)in ReSpec for which I filed an issue as well: https://github.com/darobin/respec/issues/182

@darobin, till when do you think those issue could be resolved? Could you please also have a quick look at the idlharness I created to make sure it's correct.

Thanks a lot,
Markus

lanthaler added a commit that referenced this issue Apr 2, 2013
This addresses #238.
@lanthaler
Copy link
Member Author

_Robin Berjon's reply:_

On 02/04/2013 12:48 , Markus Lanthaler wrote:

On Tuesday, April 02, 2013 4:29 AM, Sandro Hawke wrote:

So we're in the odd situation with JSON-LD-API of needing a normative
reference to WebIDL, which might stay stuck in CR for some time. In
this case, however, we can proceed to REC, even with it in CR, by
following these instructions:

You'll need to provide evidence that the WebIDL used in your spec is
stable, as follows:

[[
To provide evidence that your WebIDL is stable and tested, you could
use idlharness.js. Example at [1] and run it against Firefox nightly
and IE10. idlharness.js doesn't cover all of WebIDL. If you run into a
limitation, contact Robin Berjon and he might be able to help.

[1]
http://w3c-test.org/webperf/tests/approved/UserTiming/idlharness.html
]]

I've added an idlharness test: http://json-ld.org/test-suite/idltest/
Unfortunately it fails with the following errors:

  • callback not yet supported
  • callback not yet supported
  • callback not yet supported
  • enum not yet supported

That's not the problem you're seeing. The above output to the console is from warnings, not errors. What they mean is that idlharness will not test those constructs, but they don't prevent all the other tests to run.

The problem you have is that you haven't loaded testharness.js, and so you're getting ReferenceErrors:

ReferenceError: add_completion_callback is not defined
idlharness.js (line 1056)

ReferenceError: test is not defined
idlharness.js (line 1056)

The reason for that is that your HTML is broken:

<script src="../../playground/jsonld.js"
<script src="http://w3c-test.org/resources/testharness.js"></script>

If you fix that you should be able to get a clean run from idlharness.

I've also discovered a bug (?)in ReSpec for which I filed an issue as well:
https://github.com/darobin/respec/issues/182

Have you tried this with the canonical ReSpec instead of your local copy? I fixed a number of bugs with union type formatting a little while back. It's possible that some are still there, but I'd rather know with the real thing (local copies are evil).

Robin, till when do you think those issue could be resolved?

I don't have an ETA but updates to idlharness are being planned. In the meantime, what you should do is this:

  • Use idlharness to test what it can test.
  • Handcraft tests for what it does not yet support.
  • Have the latter be reviewed by someone who has some authority in WebIDL.

That should be enough to get out of CR (at least, it's a case I would make).

@lanthaler
Copy link
Member Author

This issue is currently blocked on digitalbazaar/jsonld.js#25. jsonld.js doesn't implement the specified interface and thus all tests fail. Currently we have no other JavaScript implementation AFAIK.

@dlongley
Copy link
Member

dlongley commented Apr 4, 2013

Fixing jsonld.js issue 25 is on my short list of JSON-LD things to do. I'll be taking care of it soon; it shouldn't be too much work.

@dlongley
Copy link
Member

dlongley commented Apr 4, 2013

@lanthaler, I've updated jsonld.js (and the playground's copy of it), but the idltest still does not fully pass. I'm pretty sure that it couldn't possibly fully pass because the result returned by typeof for a JavaScript class is always going to be '[object Object]', not what the idl parser is looking for. We could use ECMAScript 5 APIs (Object.defineProperty) to maybe avoid some of the other "writable"/"configurable"/"enumerable" errors, but I don't know if that's useful.

Furthermore, I'm not sure how the parameter count is being determined by the parser, but either it's wrong or our syntax is. I also swapped the parameter order to ensure the JsonLdCallback is last, as is common practice in JavaScript (particularly in node.js). We need to update the spec to reflect this.

Anyway, jsonld.js gets a 8/14 on the idltest now.

See: 9470356

@lanthaler
Copy link
Member Author

I've updated jsonld.js (and the playground's copy of it), but the
idltest still does not fully pass. I'm pretty sure that it couldn't
possibly fully pass because the result returned by typeof for a
JavaScript class is always going to be '[object Object]', not what the
idl parser is looking for. We could use ECMAScript 5 APIs
(Object.defineProperty) to maybe avoid some of the other
"writable"/"configurable"/"enumerable" errors, but I don't know if
that's useful.

I don't know either.. will ask Robin.

Furthermore, I'm not sure how the parameter count is being determined
by the parser, but either it's wrong or our syntax is.

Since options is optional, you have to remove it.. so

jsonld.expand = function(input, options, callback)

must become

jsonld.expand = function(input, callback)

and you retrieve the options from the arguments array. If you change this, we pass 11 of the 14 tests.

I also swapped
the parameter order to ensure the JsonLdCallback is last, as is common
practice in JavaScript (particularly in node.js). We need to update
the spec to reflect this.

That's true.. but not if one parameter is required and the other is optional. Otherwise you can't omit the options parameter. You have to always pass either an empty object or null. I think we should leave it as it is in the current spec.

@dlongley
Copy link
Member

dlongley commented Apr 4, 2013

Since options is optional, you have to remove it.

I thought that wouldn't work because I was reading the '?' in the syntax as meaning optional -- so I thought the parameter count was just incorrect. I see the word 'optional' there now and assume that's the syntax that indicates whether or not something is actually optional. I can make the WebIDL API pass those other 3 tests when I get a chance now.

That's true.. but not if one parameter is required and the other is optional. Otherwise you can't omit the options parameter. You have to always pass either an empty object or null. I think we should leave it as it is in the current spec.

You don't have to do that, you can still just omit the extra property. The function can use type checking to determine what the parameters are (this is how it's done in JS/node.js -- and the callback is always last regardless of various optional params). I'd prefer it to be last as it goes against convention otherwise (at least in JS/node.js).

cc: @msporny, @gkellogg, @niklasl

@lanthaler
Copy link
Member Author

Since options is optional, you have to remove it.

I thought that wouldn't work because I was reading the
'?' in the syntax as meaning optional -- so I thought
the parameter count was just incorrect. I see the word
'optional' there now and assume that's the syntax that
indicates whether or not something is actually optional.
I can make the WebIDL API pass those other 3 tests when
I get a chance now.

Great.

That's true.. but not if one parameter is required and
the other is optional. Otherwise you can't omit the
options parameter. You have to always pass either an
empty object or null. I think we should leave it as it
is in the current spec.

You don't have to do that, you can still just omit the
extra property. The function can use type checking to
determine what the parameters are (this is how it's done
in JS/node.js -- and the callback is always last regardless
of various optional params). I'd prefer it to be last as
it goes against convention otherwise (at least in
JS/node.js).

Yeah, you could do that (manually) but it's prohibited by the WebIDL spec unless I misread it:

An argument is considered to be an optional argument if it is declared with the optional keyword. The final argument of a variadic operation is also considered to be an optional argument. Declaring an argument to be optional indicates that the argument value can be omitted when the operation is invoked. An argument MUST NOT be declared to be optional unless all subsequent arguments to the operation are also optional.

http://www.w3.org/TR/WebIDL/#dfn-optional-argument

To make the WebIDL a bit easier to read I would propose to add two typedefs:

typedef (object or object[] or DOMString) JsonLdInput;
typedef (object or DOMString) JsonLdContext;

This doesn't change anything but makes the WebIDL much easier to read.

lanthaler added a commit that referenced this issue Apr 5, 2013
@dlongley
Copy link
Member

dlongley commented Apr 8, 2013

An argument MUST NOT be declared to be optional unless all subsequent arguments to the operation are also optional.

That's very unfortunate. We could declare the callback optional and throw an error if it isn't provided, but it is hacky. I'm not sure if that's better or worse than breaking convention; it will be surprising either way, it's just a question of which way will generate more surprises. I think my vote would be to avoid breaking convention.

@lanthaler
Copy link
Member Author

A way around this, would be to use method overloading. Unfortunately that doesn’t seem to work either as dictionaries and callbacks are “not distinguishable”.

I discussed this problem with Robin Berjon. According to him, that’s a bug in WebIDL (that he filed after our discussion: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21640). He recommends to use Futures instead of callbacks:

I actually think that you could do one better: use Futures. They're new
in terms of standards, but they're conceptually mature because we've
been thinking about them for quite a while. See:

 http://dom.spec.whatwg.org/#future

You could then have:

Future expand (JsonLdInput input, optional JsonLdOptions options);

Which in code would be:

 proc.expand(input).then(successCB, errorCB);

Futures have a lot of advantages, amongst them being composable (so as
to avoid having a tangled mess of async callbacks all over) and
providing a common API through the platform.

The goal is to move everyone to Futures.

That would mean, however, that we would introduce a normative dependency on DOM - which I think, we don't wanna do at this stage.

@lanthaler
Copy link
Member Author

RESOLVED: Express the WebIDL for functions with optional parameters using the method overloading WebIDL pattern.

lanthaler added a commit that referenced this issue Apr 9, 2013
lanthaler added a commit that referenced this issue Apr 9, 2013
lanthaler added a commit to lanthaler/json-ld.org that referenced this issue Oct 21, 2013
lanthaler added a commit to lanthaler/json-ld.org that referenced this issue Oct 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants