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

Error on a constant declaration for HTMLAudioElement #3488

Closed
dascritch opened this issue Oct 23, 2019 · 6 comments
Closed

Error on a constant declaration for HTMLAudioElement #3488

dascritch opened this issue Oct 23, 2019 · 6 comments
Labels
internal-issue-created An internal Google issue has been created to track this GitHub issue triage-done Has been reviewed by someone on triage rotation.

Comments

@dascritch
Copy link

When I have an audiotag typed as an HTMLAudioElement, and i'm checking audiotag.readyState < audiotag.HAVE_CURRENT_DATA , closure in ADVANCED_OPTIMIZATIONS mode doesn't like it :

WARNING - [JSC_INEXISTENT_PROPERTY] Property HAVE_CURRENT_DATA never defined on HTMLAudioElement

As I'm reading, HAVE_CURRENT_DATA is defined here : https://github.com/google/closure-compiler/blob/master/externs/browser/html5.js#L1944
But not on HTMLMediaElement.prototype . So we cannot even use HTMLAudioElement.HAVE_CURRENT_DATA.

As I can read in https://developer.mozilla.org/en-US/docs/Web/API/HTMLAudioElement :

inherits properties from its parent, HTMLMediaElement, and from HTMLElement. 

Perhaps I'm wrong, but as any browsers is accepting my audiotag.HAVE_CURRENT_DATA, may be those declarations must be changed into :

HTMLMediaElement.prototype.HAVE_CURRENT_DATA;
@kjin
Copy link
Contributor

kjin commented Oct 23, 2019

Created Google internal issue http://b/143219987

@kjin kjin added internal-issue-created An internal Google issue has been created to track this GitHub issue triage-done Has been reviewed by someone on triage rotation. labels Oct 23, 2019
@kjin
Copy link
Contributor

kjin commented Oct 23, 2019

Is there any particular reason you can't write it as audiotag.readyState < HTMLMediaElement.HAVE_CURRENT_DATA? It's true that in browsers you can access it on any of HTMLMediaElement or its subclasses' definitions or prototypes, but given that it is a constant, would make more sense to access it as a static property of the base class itself?

@dascritch
Copy link
Author

Hi @kjin.
Honestly, no, there is no technical reason, except losing some chars in the outputted minified JS, as we will have :

a.readyState<HTMLMediaElement.HAVE_CURRENT_DATA

instead of

a.readyState<a.HAVE_CURRENT_DATA

It may count when I have lots of references to this kind of constants.

I know, this is really ugly and over-engineering

@dascritch
Copy link
Author

@kjin
Copy link
Contributor

kjin commented Oct 24, 2019

Ah, got it. I do see that the HTML5 externs file does contain existing examples where constants exist on both a class and its prototype (like WebSocket) so this doesn't seem like an unprecedented change, it seems doable.

@kjin
Copy link
Contributor

kjin commented Oct 25, 2019

I've landed a change to add all of the constants on HTMLMediaElement to its prototype as well ( (should work for instances of type HTMLAudioElement too), and it should be in the next compiler release.

@kjin kjin closed this as completed Oct 25, 2019
shicks pushed a commit that referenced this issue Oct 29, 2019
This addresses #3488.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=276597408
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-issue-created An internal Google issue has been created to track this GitHub issue triage-done Has been reviewed by someone on triage rotation.
Projects
None yet
Development

No branches or pull requests

2 participants