Skip to content
This repository has been archived by the owner on Jan 24, 2023. It is now read-only.

Revisit/refactor MySky singleton #374

Open
mrcnski opened this issue Jan 13, 2022 · 1 comment
Open

Revisit/refactor MySky singleton #374

mrcnski opened this issue Jan 13, 2022 · 1 comment

Comments

@mrcnski
Copy link
Contributor

mrcnski commented Jan 13, 2022

I'm also not really a fan of the Error thrown in the MySky constructor. I would make it possible for multiple MySky instances to be created, but structure the code in a way that only one is ever created. That's more easily said than done of course but perhaps in this case the easiest thing to do is use a global variable.

What's the reason only one MySky instance should get created by the way? Perhaps there's no real good reason and we don't have to care about all of this.

Originally posted by @peterjan in #361 (comment)

@mrcnski mrcnski mentioned this issue Jan 13, 2022
5 tasks
@mrcnski
Copy link
Contributor Author

mrcnski commented Feb 11, 2022

    if (MySky.instance) {
      return MySky.instance
    }

    this.pendingPermissions = permissions;
    MySky.instance = this
  }

Setting MySky.instance is also a responsibility of the constructor and not of New because using New is optional and someone can easily do

const ms1 = new MySky(...)
const ms2 = new MySky(...)

and it will just work. Also returning the existing instance is the standard practice with factories and other singleton implementations (the usual is a getInstance() as PJ mentioned).

(We'll also need to change the MySky.instance = new MySky(connector, permissions, hostDomain, currentPortalUrl); on line 217.)

I just read the discussion we had about this a month ago - I had forgotten about it. The way I see it we have two options:

  1. Stop calling this a singleton and make is a standard class.
  2. Implement a proper singleton.

I guess either one works and neither will take very long. I believe the suggested code above solves all issues we have with the singleton approach - it ensures that only one MySky object can be created and it also doesn't thow the error which PJ didn't like.

Originally posted by @ro-tex in #361 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant