web icon indicating copy to clipboard operation
web copied to clipboard

Handle `LegacyFactoryFunction`s in generator

Open srujzs opened this issue 1 year ago • 7 comments

Some interfaces in the Web IDL have a LegacyFactoryFunction defined in their metadata e.g. HTMLAudioElement that tells us that the factory for that interface is defined differently e.g. HTMLAudioElement here: https://html.spec.whatwg.org/multipage/media.html#the-audio-element.

The surface area of this metadata is small, affecting 3 interfaces today, all of which are elements and can be instantiated using createElement.

srujzs avatar Dec 07 '23 22:12 srujzs

Copying your reply from https://github.com/flutter/flutter/issues/139125 :

This is a bug in package:web. I filed an issue here: https://github.com/dart-lang/web/issues/124. The workaround is simple for now: document.createElement('audio').

There are no custom elements involved in package:web. It lowers the constructor to the intuitive lowering new HTMLAudioElement() in JS. That JS code returns that error, since the working way is to do new Audio() instead.

Marking it closed and tracking it in the package instead.

Now is this a future compatible workaround, in the sense of, wiil the document.createElement('audio') approach continue to work once web addresses this internally in the future?

ryanheise avatar Dec 08 '23 01:12 ryanheise

Yes, it should. createElement is a key DOM API, so it should not change even if we fix this issue.

srujzs avatar Dec 08 '23 01:12 srujzs

Ah, got it. In my head, I was completely off track in what I had thought you meant.

Now that I understand what you meant, I guess my question just to confirm is, can I assume that the typecast document.createElement('audio') as HTMLAudioElement or document.createElement('audio') as AudioElement is future compatible?

ryanheise avatar Dec 08 '23 01:12 ryanheise

AudioElement is a typedef that we made to make it easier to migrate off of dart:html. At some far future date, we'd like to deprecate and remove that "glue" layer when it's no longer needed. So, I'd prefer the first one if you're worried about future compatibility, which should continue to work.

It's worth noting that this package is still not at v1, mostly because we want to keep adding features and migrate to extension types before we do so. document.createElement('audio') as HTMLAudioElement will still almost certainly not break, but something to keep in mind. Of course, if a breaking change comes along, we will release a breaking version, so your code should continue to work unless you update your pubspec.

srujzs avatar Dec 08 '23 02:12 srujzs

I had the same issue for

  final HTMLMetaElement metaElement = HTMLMetaElement()
    ..name = 'google-signin-client_id'
    ..content = flavor.googleWebClientId;

Is this issue for all HTML elements or audio element only?

amrgetment avatar Feb 21 '24 11:02 amrgetment

Is this issue for all HTML elements or audio element only?

The issue states that it's not all, not one, but exactly "three". The surface area is small. (HTMLMetaElement is not one of them.)

ryanheise avatar Feb 21 '24 12:02 ryanheise

@ryanheise is correct here: this is a different bug even though it has similar results. Responded in #183.

srujzs avatar Feb 21 '24 17:02 srujzs

@srujzs Based on the following description for [LegacyFactoryFunction] values,

If the [LegacyFactoryFunction] extended attribute appears on an interface, it indicates that the JavaScript global object will have a property with the specified name whose value is a function that can create objects that implement the interface. From IDN docs

Would you say the following would be a correct representation of such factory functions in dart?

HTMLAudioElement Audio(String src){
  return HTMLAudioElement()..src =src;
}

Are there any other specifics that we want to take into account if we have to add support for [LegacyFactoryFunction] in the generator?

rutvik110 avatar Sep 09 '24 17:09 rutvik110

I actually don't think this is needed anymore. These are legacy ways to construct 3 element types. We added default constructors to call document.createElement instead for these and other element types, and therefore, there's no need to call these legacy constructors. I think we can go ahead and close this as the original motivation is handled.

srujzs avatar Sep 10 '24 00:09 srujzs