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

Unroll argument conversion loop (for non-... args) #5

Closed
domenic opened this issue Sep 27, 2015 · 6 comments
Closed

Unroll argument conversion loop (for non-... args) #5

domenic opened this issue Sep 27, 2015 · 6 comments

Comments

@domenic
Copy link
Member

domenic commented Sep 27, 2015

  const args = [];
  for (let i = 0; i < arguments.length && i < 3; ++i) {
    args[i] = arguments[i];
  }
  args[0] = conversions["DOMString"](args[0]);

could become

args[0] = conversions["DOMString"](arguments[0]);
args[1] = arguments[1];
args[2] = arguments[2];

This also helps with #4 since it automatically fills out args[2] with undefined if it wasn't passed.

@Sebmaster
Copy link
Member

I guess this is kinda related to #4. I'm still hanging on to that thought of ensuring arguments.length equivalence for impl classes.

@domenic
Copy link
Member Author

domenic commented Sep 27, 2015

I am pretty sure impl classes should not rely on arguments.length though. I don't think WebIDL even allows that checking. It is all handled in the WebIDL layer.

Sebmaster referenced this issue in jsdom/jsdom Oct 12, 2015
@Sebmaster
Copy link
Member

In order to fix the dictionary thing, arguments.length has been broken already, so we won't need a major for further changes there, but I haven't unrolled the loop yet since we need to do a bit of conditionals there to support variadic args.

@domenic
Copy link
Member Author

domenic commented Jan 5, 2016

I notice this kind of happened but there's a tryImplForWrapper loop above the unrolled conversions now. I think we should be able to apply tryImplForWrapper only to (known?) interface types.

@Sebmaster
Copy link
Member

I think we should be able to apply tryImplForWrapper only to (known?) interface types.

Not right, now, since we never know about all the interface types. Take Node.dispatchEvent for example. We don't actually know about the Event type there (in jsdom, due to our folder structure at least, since we convert all folders seperately).

I plan to update the conversion interface to include more state so you can add multiple folders to the same conversion process, so we'd know about the whole interface system, but haven't gotten to it yet.

TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Feb 8, 2017
TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Feb 8, 2017
@TimothyGu
Copy link
Member

TimothyGu commented Aug 5, 2017

An update: since about three months ago (in #33) we no longer try to get the impl if the type is known not to be an interface (sequences, records, promises, primitives). There is still the major pain point that we don't yet have a way to import types from other modules. E.g. the URL interface is implemented in whatwg-url using webidl2js, but the webidl2js in jsdom doesn't know URL is an interface, and even though it still tries unwrapping it doesn't work because the impl symbol is different for interfaces in another module.

I've got a fix in the works, but will have to change up the type system a bit here so that it doesn't assume the impl symbol is the same for all interfaces. I'll also have to think of a solution for hybrids like Window, in that it functions like a generated class and has impl property (because it inherits EventTarget), but is not actually generated.

TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Oct 15, 2017
TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Oct 15, 2017
TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Oct 15, 2017
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

No branches or pull requests

3 participants