c-hive / guides

Guides for code style, way of working and other development concerns
MIT License
26 stars 5 forks source link

Best practice for JS Singleton pattern #39

Closed thisismydesign closed 4 years ago

thisismydesign commented 4 years ago

It isn't obvious how to do it right: https://stackoverflow.com/questions/1479319/simplest-cleanest-way-to-implement-singleton-in-javascript

We're using the ES6 class approach but it would be better if we'd use .instance or .instance() to make the pattern obvious for readers. Also, when to freeze the object should also be clarified.

gomorizsolt commented 4 years ago

Yea, seems to be a controversial topic. How about this approach btw?

class Foo {
   // ...
}

class Singleton {
   static instance = null;

   static getInstance() {
      if (!this.instance) {
         this.instance= new Foo();
      }

      return this.instance;
   }
}

const s1 = Singleton.getInstance();
const s2 = Singleton.getInstance();

console.log(s1 === s2);
// => true, because both classes point to the same reference

It's akin to java's approach despite the fact Singleton can be instantiated because there's no obvious way to make constructors private in js.

gomorizsolt commented 4 years ago

@thisismydesign would you like to apply the above-mentioned proposal to the projects where it's relevant? Or let's hold off this decision for now?

thisismydesign commented 4 years ago

Looks good. Although it doesn't prevent from misusing the pattern (e.g. accessing instance directly) when used correctly this is a good approach IMO. I will add it to the guides too.

thisismydesign commented 4 years ago

Note, there are also private static fields: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Class_fields

gomorizsolt commented 4 years ago

Yes, but that's still an experimental feature(stage 3), see https://github.com/tc39/proposal-class-fields.

thisismydesign commented 4 years ago

Both public and private though:

Both Public and private field declarations are an experimental feature (stage 3)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Class_fields

thisismydesign commented 4 years ago

So we switched to another approach that seems to be working well. @gomorizsolt could you add it to the guides and resolve this issue?

gomorizsolt commented 4 years ago

Whoa, I can't recall this topic anymore. What's our preferred approach right now?

class Singleton {
   static instance = null;

   static getInstance() {
      if(!this.instance) {
         this.instance = new Foo();
      }

      return this.instance;
   }
}

AFAICT it's identical to https://github.com/c-hive/guides/issues/39#issuecomment-575040514.

thisismydesign commented 4 years ago

Whatever we use in Sniper should be the currently preferred approach.

gomorizsolt commented 4 years ago

Resolved by #43.