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

New principle: Naming of factory methods #378

Closed
LeaVerou opened this issue Jul 18, 2022 · 7 comments · Fixed by #471
Closed

New principle: Naming of factory methods #378

LeaVerou opened this issue Jul 18, 2022 · 7 comments · Fixed by #471
Assignees
Labels
Agenda+ tc39-tracker Issues where TC39 input would be useful Topic: JS

Comments

@LeaVerou
Copy link
Member

LeaVerou commented Jul 18, 2022

This was brought up in our discussion of Response.json() in w3ctag/design-reviews#741

We should have guidelines for naming factory methods. Currently the web platform is pretty inconsistent:

  • document.createXXX() for creating nodes, also document.createRange(), document.createTreeWalker() etc
  • the deprecated document.initXXXEvent() factory methods
  • The Temporal API uses from() methods
  • createImageBitmap() which is oddly placed in the global scope (?!)
  • Object.create()
  • CanvasRenderingContext2D#createXXX()
  • Array.from()
  • Object.fromEntries()
  • String.fromCharCode()
  • audioContext.createXYZ()
  • Response.json()
  • DOMQuad.fromRect()

Perhaps we should have guidance that factory methods should start with create or from to make it obvious that they are factory methods? Not sure we should restrict it further, as these are not interchangeable.

Though there are also things like Date.parse(), which are fine as-is…

@ptomato
Copy link
Contributor

ptomato commented Jul 18, 2022

For Temporal we took our from name from other factory methods of JS builtin objects, e.g. Array.from(), Object.fromEntries(), String.fromCharCode(). Object.create() is the oddball, I think.

@domenic
Copy link
Member

domenic commented Jul 19, 2022

IMO there are several categories of things here:

  • Constructors. These should be used for the "most general" creation of a class, i.e. the one that specifies the raw data. E.g. new Response() or new DOMQuad().

  • Factory functions. These should be used for specialized shortcuts to creating an object, essentially sugar over the constructors. They should live as static methods on the constructor. E.g. Response.json() or DOMQuad.fromRect().

  • Misplaced factory functions. These are mostly legacy from the bad old days when people didn't use constructors. They live on some other class. E.g. document.createXYZ() or audioContext.createXYZ().

And then the async variants:

  • Async constructors. These are similar to constructors---they are the main entry point for creating a class---but because class instances must not be created in a sync way, we need to return a promise, and thus we cannot use new. There is no clear guidance for this that I know of, and not very many examples; createImageBitmap() might be the only one. Guidance would be good as I feel this has come up in the past. I kind of like X.create(), but I haven't thought about this too much. I think it's OK for the namespace here to overlap with factory functions, personally.

  • Async factory functions. Similar to sync factory functions, but even fewer examples. CropTarget.fromElement() might fit in here, or might be an async constructor, not sure.

Guidance here should probably be integrated with https://w3ctag.github.io/design-principles/#constructors .

@js-choi
Copy link

js-choi commented Jul 20, 2022

In addition to the categories that @domenic mentioned, Ecma262 JavaScript also has some “coercion” functions that are constructors being called as if they were ordinary functions, without new. For example, Integer(v).

The iterator-helpers proposal also uses this pattern: Iterator is a constructor, but Iterator(o) (an ordinary function call) attempts to “coerce” the given input into an Iterator object.

@torgo torgo added this to the 2022-10-03-week milestone Oct 2, 2022
@cynthia cynthia self-assigned this Nov 8, 2022
@torgo torgo modified the milestones: 2022-11-07-week, 2022-12-05-week Dec 4, 2022
@silverwind
Copy link

Should it not be forbidden to use the same name of an instance method (Response.prototype.json) and static methods (Response.json)?

Take for example the MDN Response document. They just omit the .prototype part of the names, so they could not acutally create two pages with their current URL scheme.

@domenic
Copy link
Member

domenic commented Dec 12, 2022

MDN is working on fixing their unfortunate URL-scheme choices from many years ago, and such unfortunate choices should not constrain web platform API design.

@LeaVerou
Copy link
Member Author

Agreed with @domenic. Also, naming static and instance methods the same can actually be a very good practice in certain cases where a method does the same thing, the instance one operating on the instance itself, and the static one on the first parameter (though I can't think of an example of that off the top of my head).

@LeaVerou
Copy link
Member Author

The original PR (#471) included this sentence:

An example of valid usage of a factory method is
when an object is being created it also requires association
with the parent object.
document.createXXX() is an example of this.

I think this is actually an antipattern that we should actively discourage. This should have been Element.create({ document }), because:

  1. It’s a pattern that does not scale: what happens when you need to associate with two objects?
  2. It does not make it clear what object is created. With factory methods being static methods on a class, even when a subclass is returned, at least we know what the base class is, whereas something like document.createXXX() could return anything.
  3. It introduces an undesirable coupling between two classes when all we want is a relationship. E.g. what if in the future we want elements that are not associated with a document?
  4. It’s violates most precedent (the vast majority of factory methods on the web platform are static methods).

Furthermore, the way this is phrased is inaccurate: for DOM elements the parent object is element.parentNode, not document. Which highlights another problem with this pattern.

OTOH, things like CanvasRenderingContext2D#createXXX() seem more reasonable (though static methods would also not have been terrible), so the guidance should probably be more nuanced than "never do this".

LeaVerou added a commit that referenced this issue Jan 25, 2024
* Add guidance for factory method naming

Resolves #378.

* Update index.bs

Co-authored-by: Amy Guy <[email protected]>

* Update index.bs

Co-authored-by: Amy Guy <[email protected]>

* Update index.bs

* Update index.bs

Co-authored-by: Amy Guy <[email protected]>

* Update index.bs

Co-authored-by: Amy Guy <[email protected]>

* Update index.bs

Co-authored-by: Lea Verou <[email protected]>

* Update index.bs

Co-authored-by: Sangwhan "fish" Moon <[email protected]>

* Update index.bs

* Update index.bs

* Update index.bs

---------

Co-authored-by: Amy Guy <[email protected]>
Co-authored-by: Lea Verou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agenda+ tc39-tracker Issues where TC39 input would be useful Topic: JS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants