Skip to content

Conversation

@karlseguin
Copy link
Collaborator

HTMLImageElement is the correct class name. However, it has a "legacy factory": Image (i.e. new Image()).

HTMLImageElement is the correct class name. However, it has a "legacy factory":
Image (i.e. new Image()).
Copy link
Contributor

@sjorsdonkers sjorsdonkers left a comment

Choose a reason for hiding this comment

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

Good to know, thanks!

See Alias consideration

try parser.imageSetIsMap(self, is_map);
}

pub const Factory = struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about calling it Alias?

Suggested change
pub const Factory = struct {
pub const Alias = struct {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not quite an alias though, not per the spec and not how it's implemented. The most obvious practical difference is:

new Image() // valid;
new HTMLImageElement() // invalid...which I find ridiculous, but...

But, also, HTMLImageElement != Image

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it's a strong typedef/alias, I also have seen it be called distinct variant, but I don't like that one.
Factory is somewhat implying it is creating an HTMLImageElement.
Up to you :) 🤷 I'd probably go with StrongAlias or Alias (and have the strong part be implicit). Probably StrongAlias as that would encode the knowledge of your reply.

@karlseguin karlseguin merged commit 5909ab7 into main May 30, 2025
11 checks passed
@karlseguin karlseguin deleted the fix_html_image branch May 30, 2025 14:21
@github-actions github-actions bot locked and limited conversation to collaborators May 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants