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

Symbol in the module exports array causes fatal error #1246

Closed
andrew-yustyk opened this issue Oct 31, 2018 · 11 comments
Closed

Symbol in the module exports array causes fatal error #1246

andrew-yustyk opened this issue Oct 31, 2018 · 11 comments

Comments

@andrew-yustyk
Copy link
Contributor

andrew-yustyk commented Oct 31, 2018

I'm submitting a...


[ ] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Currently it isn't possible to use symbol in the exports array.
An example:

// sentry.constants.ts
export const SENTRY_SDK_TOKEN = Symbol('SENTRY_SDK_TOKEN');
// sentry.module.ts
@Module({
  imports: [ConfigModule],
  providers: [
    {
      provide: SENTRY_SDK_TOKEN,
      useFactory: SentryProvider.fromConfigService,
      inject: [ConfigService]
    }
  ],
  exports: [SENTRY_SDK_TOKEN]
})
export class SentryModule {
}

The bug is not reproduced if you use the string instead of the symbol.

Seems like this is a bug in the addExportedComponent() method in the injector module (link).

Current implementation of this method doesn't have a checking of the symbol token, just for the string token.

Expected behavior

Symbol token can be used in the exports section as well as the string token

Environment


Nest version: 5.4.0

For Tooling issues:
- Node version: 10.11.0
- Platform:  Linux
@marcus-sa
Copy link

marcus-sa commented Oct 31, 2018

Nest doesn't support using Symbol as a token yet.
You need to use the good ol' strings.
It's already in the roadmap :)

@andrew-yustyk
Copy link
Contributor Author

Nest supports symbols from 5.3.12
Support was added in Issue #1177

@andrew-yustyk
Copy link
Contributor Author

There is just a bug or missed functionality. I'll try to create PR for for this issue asap

@marcus-sa
Copy link

Oh, my bad then

@andrew-yustyk
Copy link
Contributor Author

there is need just to update addExportedComponent() method in in the injector module

@marcus-sa
Copy link

Anyhow, you should use Symbol.for and not Symbol

@andrew-yustyk
Copy link
Contributor Author

I think this is depends on your project.
I prefer to use Symbol('some_str') instead of Symbol.for('some_str').
Symbol.for allows other developers do not import the constant with symbol and just use Symbol.for in different places of project. And then an additional time will be spent during refactoring.

In a case of Symbol other developers must import the constant with this symbol.

@marcus-sa
Copy link

marcus-sa commented Oct 31, 2018

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/for
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol

There's a big difference.
You need to use Symbol.for, because EVERY instantiated symbol using Symbol will be unique, so it's impossible to assert it.

Symbol.for('foo') === Symbol.for('foo'); // true
Symbol('foo') === Symbol('foo'); // false

This is exactly the same behavior with InjectionToken in Angular.
That's just an instantiated class with a description, so if you'd create two InjectionToken with the same description, it'd still assert as they're the same, which is exactly the whole point, lol.
You use a prefix when building a library, to make sure no one will create an identical token.

@kamilmysliwiec
Copy link
Member

Thanks for reporting!

@kamilmysliwiec
Copy link
Member

Published as 5.4.1

@lock
Copy link

lock bot commented Sep 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants