Inversifyjs: Use Symbol.for(key) instead of Symbol(key) everywhere

[remojansen]

Expected Behavior

We should use in all our code / examples / docs (and recommend using) Symbol.for(key) to create new symbols instead of Symbol(key) because it is unsafe!

Current Behavior

We currently use in all our code / examples / docs (and recommend using) Symbol(key) to create new symbols. This is unsafe!

Steps to Reproduce (for bugs)

The following code snippet demonstrates why it is unsafe:

ts-node temp.ts 

#

function test1() {
    const m = new Map<symbol, number>();

    for (let i = 0; i < 50; i++) {
        const key = Symbol.for(`KEY_${i}`);
        const value = i;
        m.set(key, value);
    }

    for (let i = 0; i < 50; i++) {
        const key = Symbol.for(`KEY_${i}`);
        const value = m.get(key);
        if (value === undefined) {
            throw new Error(`Cannot get ${key.toString()}`);
        }
    }

    console.log("TEST 1 OK :)");
}

function test2() {
    const m = new Map<symbol, number>();

    for (let i = 0; i < 50; i++) {
        const key = Symbol(`KEY_${i}`);
        const value = i;
        m.set(key, value);
    }

    for (let i = 0; i < 50; i++) {
        const key = Symbol(`KEY_${i}`);
        const value = m.get(key);
        if (value === undefined) {
            throw new Error(`Cannot get ${key.toString()}`);
        }
    }

    console.log("TEST 1 OK :)");
}

test1();
test2();

Context

Using Symbol(key) is unsafe because the symbols are not unique!

From MDN about Symbol:

To create a new primitive symbol, you write Symbol() with an optional string as its description:

var sym1 = Symbol();
var sym2 = Symbol('foo');
var sym3 = Symbol('foo');

The above code creates three new symbols. Note that Symbol("foo") does not coerce the string "foo" into a symbol. It creates a new symbol each time:

Symbol('foo') === Symbol('foo'); // f

And MDN about Symbol.for(key):

The Symbol.for(key) method searches for existing symbols in a runtime-wide symbol registry with the given key and returns it if found. Otherwise a new symbol gets created in the global symbol registry with this key.

This has created a real-world issue in one of my projects.

We need PRs for all the projects:

• [x] https://github.com/inversify/InversifyJS
• [x] https://github.com/inversify/inversify-express-utils
• [x] https://github.com/inversify/inversify-express-example
• [x] https://github.com/inversify/inversify-basic-example
• [x] https://github.com/inversify/inversify-vanillajs-helpers
• [x] https://github.com/inversify/inversify-inject-decorators
• [x] https://github.com/inversify/inversify-logger-middleware
• [x] https://github.com/inversify/inversify-binding-decorators
• [x] https://github.com/inversify/inversify-restify-utils

Your Environment

• [email protected]
• [email protected]

Stack trace

TEST 1 OK :)

/home/rjansen/CODE/core/temp.ts:34
            throw new Error(`Cannot get ${key.toString()}`);
                  ^
Error: Cannot get Symbol(KEY_0)
    at test2 (/home/rjansen/CODE/core/temp.ts:34:19)
    at Object.<anonymous> (/home/rjansen/CODE/core/temp.ts:42:1)
    at Module._compile (module.js:569:30)
    at Module.m._compile (/usr/lib/node_modules/ts-node/src/index.ts:406:23)
    at Module._extensions..js (module.js:580:10)
    at Object.require.extensions.(anonymous function) [as .ts]
        (/usr/lib/node_modules/ts-node/src/index.ts:409:12)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)

[matthewjh]

@remojansen: what exactly is the problem with using Symbol("a")? The reason you were using symbols in the first place was to prevent collisions, no? Using Symbol.for seems little different to using strings, i.e. it collides.

[remojansen]

@matthewjh The problem is that every time you call Symbol("XXXX") you get a new unique symbol. For example, the following won't work:

container.bind<UserRepository>(Symbol("UserRepository")).to(UserRepository)

@inject(Symbol("UserRepository"))

The above doesn't work because the symbols created in the first and second call of Symbol("XXXXX") are two unique symbols.

We normally don't face this problem because we declare the symbol once in advance but I have experienced a problem in one project in which we load and unload modules dynamically and we declare symbols dynamically. Using Symbol.for("XXXXX") will always return the same symbol and not cause any problems so it is safer.

[matthewjh]

@remojansen: yes, that's right. I presumed people were only using Symbols by declaring them in advance, as you said.

But how is Symbol.for("XXXX") any different to just using "XXXX" (as a string)? I thought the only justification for using Symbols with inversify was the very fact that they're unique (no collisions), hence my confusion over the PR. :P

[remojansen]

A small application can probably work just fine with strings as IDs. As the application grows and especially if you have very large teams chances of two classes with the same name will increase and Symbol will prevent these collisions if on top of that you are doing dynamic stuff Symbol.for will protect you.

If you don't want to worry about this the best thing to do is to use Symbol.for and then you are safe for the future growth of your app.

[matthewjh]

@remojansen

How will `Symbol.for" protect you from collisions, though? If you call Symbol.for twice with the same string in the same JS context, you're going to get back the exact same symbol:

Symbol.for("a") == Symbol.for("a") // true

So I don't see how it provides any collision protection over strings. If you have a large team with two classes with the same name, and each team uses Symbol.for("CLASSNAME"), these two symbols will be identical just as if strings were used.

[matthewjh]

Or are you saying that people can easily switch from Symbol.for to Symbol as and when they encounter collisions?

[remojansen]

Yes, they should. At least I was able to do it without problems. Do you have any thoughts about why it could be a problem? Maybe I'm missing something?

[matthewjh]

@remojansen: that would work. I guess I don't see the point in using Symbol.for over a string in that case.

const TYPES = { MyType: "MyType" };

can easily be changed to

const TYPES = { MyType: Symbol("MyType") };

if uniqueness is desired and vice-versa.

Using a string is well-understood; using Symbol.for is more obscure and less clear. People may confuse why you're using Symbol.for over a string, just as I did, given their functional equivalence. Unless you're aiming for uniqueness, I would use strings. And if you are aiming for uniqueness, then use Symbol("name"). That's clearer, IMO.

[lillallol]

@remojansen

We normally don't face this problem because we declare the symbol once in advance but I have experienced a problem in one project in which we load and unload modules dynamically and we declare symbols dynamically.

Can you please provide a minimal reproducible example on that ?

Edit : Also why not use strings and go for Symbol.for() for the dynamic abstractions ? What is that thing that Symbol.for is offering you more than string ?