noConstructorReturn doesn't make sense in ES6

[GeorgeTailor]

Environment information

since `Rome 11.0.0`

What happened?

The default rome lint rule https://docs.rome.tools/lint/rules/noconstructorreturn/
does not make sense in ES6, which is marking creation of OOP-style singletons as incorrect. Blindly copy-pasting rules from eslint, imo, is not a good idea.

Consider the following code:

export class NotePositionService {
	static instance: NotePositionService;
	constructor() {
		if (NotePositionService.instance) {
			return NotePositionService.instance;
		}
		NotePositionService.instance = this;
		// some heavy initiazer logic, which should be run only once
	}
}

Sadly, #4005 turned into a noop.

Expected result

https://docs.rome.tools/lint/rules/noconstructorreturn/
should not be the default.

[Conaclos]

Hi @GeorgeTailor

Have you more pointers talking about this pattern?
It seems to me a strange one: Calling a constructor allocates memory, and you expect new NotePositionService() to return a new instance.

Singletons are often implemented using a factory method:

export class NotePositionService {
	private static instance: NotePositionService | null = null;

	static getInstance(): NotePositionService {
		if (NotePositionService.instance) {
			NotePositionService.instance = new NotePositionService();
		}
		return NotePositionService.instance;
	}

	private constructor() {
		// some heavy initiazer logic
	}
}

Or making the constructor inaccessible:

class NotePositionService {
	static instance: NotePositionService;

	constructor() {
		// some heavy initiazer logic
	}
}

export function instance(): NotePositionService {
	if (NotePositionService.instance) {
		NotePositionService.instance = new NotePositionService();
	}
	return NotePositionService.instance;
}

export type { NotePositionService }

[GeorgeTailor]

whether the code returns a new instance or an existing one is none of the consumer's concern, that's the whole point of creating a singleton.
Since in Javascript you cannot mark a constructor as private (using # is forbidden on constructor), you're forced to use this kind of logic in your code.
Typescript is artificial, in runtime the private constructor does not exist, it's just a constructor, so if your code is used in pure JS, there's nothing stopping the consumer to call a constructor.

[ematipico]

Hey @GeorgeTailor , I am still interested in this question:

Have you more pointers talking about this pattern?

You briefly explained the singleton pattern, but we know that this can be achieved with different techniques.

• Do you agree that this rule, to a certain degree makes sense?
• Do you think that using a suppression in your code, with a safety explanation is too much to maintain in your codebase?

@Conaclos do you think this should be removed from the recommended set?

[GeorgeTailor]

I am not sure what do pointers mean in this particular case. Examples? Use-cases? They will mostly come from backend languages where this pattern is used all over the place, especially Java + Spring with its dependency injection. But neither a "proper" implementation of a singleton nor dependency injection is possible in Javascript due to the nature of the language. Proper dependency injection is not possible in Typescript as well, despite being a compiled superset of JS, you still need to apply a bunch of hacks here and there, but that's a separate topic.

• yes, to a certain degree it makes sense (where you write hackish code that returns something else than this in constructor), as well as to a certain degree it does not, like with the singleton case.
• depends on the codebase, if a singleton pattern is a good fit for a particular app, then there will be a lot of this, supressing linter for a perfectly valid use-case is... strange.

We might debate that this rule should not apply to contructor, well... because it's just a syntactic sugar over a Function, which can return anything, but that will lead into a much broader discussion.

IMO, linter should not try to fix the language, but rather the code that is written. If a linter cannot discern between creating a singleton or just writing dumb code then it should not do anything here at all.

[Conaclos]

@GeorgeTailor

Am I wondering: how many diagnostics did you get in your codebase?

do you think this should be removed from the recommended set?

For now, I don't think so. If we get more complains, we could revisit this choice.
I think that the current pattern is not widely spread. In fact, I never see a singleton written in this way until now.
In general either a Java-like pattern is used (using a sttic member to retrieve the singleton) or a more JS-idiomatic pattern (such as directly declaring a literal object with methods const SINGLETON_INSTANCE = { ... method() {} }.
Moreover, I think that most devs has the mental model that new Something allocates a new instance.
In this case, I think this could be worth to require a comment (and thus a rome-ignore comment) to highlight this unfamiliar pattern.