fix(compiler): don't throw when Symbol is used as DI token #13701

DzmitryShylovich · 2016-12-29T12:25:50Z

vicb · 2016-12-29T16:40:06Z

modules/@angular/compiler/src/compile_metadata.ts

@@ -90,6 +90,9 @@ export function identifierName(compileIdentifier: CompileIdentifierMetadata): st
  if (ref instanceof StaticSymbol) {
    return ref.name;
  }
+  if (typeof ref === 'symbol') {
+    return ref.toString().replace(/\(|\)/g, '_');


But Symbol("foo") !== Symbol("foo") - we should probably use a (Weak)Map ?

But Symbol("foo") !== Symbol("foo")

we can add a counter to ensure uniqueness

${ref.toString().replace(/$|$/g, '_')}${counter++}

use a (Weak)Map ?

what will be the key?

we can add a counter to ensure uniqueness

Then you could not override the token, no ? (same Symbol will generate 2 different IDs)

what will be the key?

the Symbol instance ?

DzmitryShylovich · 2016-12-29T19:38:32Z

@vicb fixed

vicb · 2016-12-29T19:38:49Z

modules/@angular/compiler/src/compile_metadata.ts

+    if (symbols.has(ref)) {
+      return symbols.get(ref);
+    }
+    const symbolStr = `${ref.toString().replace(/\(|\)/g, '_')}${symbolIndex++}`;


could you make it _symbol_${_sanitizeIdentifier(ref.toString())}_${symbolIndex++}

vicb · 2016-12-29T19:39:35Z

modules/@angular/compiler/test/integration_spec.ts

+
+         TestBed.configureTestingModule({
+           declarations: [CmpWithSymbol],
+           providers: [{provide: SOME_SYMBOL, useValue: 'value'}]


and an override ?

DzmitryShylovich · 2016-12-29T20:01:45Z

@vicb done

vicb · 2016-12-29T22:36:39Z

modules/@angular/compiler/test/integration_spec.ts

+         TestBed.configureTestingModule({
+           declarations: [CmpWithSymbol],
+           providers: [
+             {provide: SOME_SYMBOL, useValue: 'value'},


{provide: SOME_SYMBOL, useValue: 'value'}, {provide: SOME_SYMBOL, useValue: 'override'},

DzmitryShylovich · 2016-12-30T08:50:01Z

@vicb comment is addressed

vicb · 2017-01-03T18:47:57Z

modules/@angular/compiler/src/compile_metadata.ts

@@ -81,6 +81,8 @@ function _sanitizeIdentifier(name: string): string {
 }

 let _anonymousTypeIndex = 0;
+let symbolIndex = 0;
+const symbols = new Map<Symbol, string>();


please rename to symbolIds

vicb · 2017-01-03T18:48:13Z

modules/@angular/compiler/src/compile_metadata.ts

+    if (symbols.has(ref)) {
+      return symbols.get(ref);
+    }
+    const symbolStr = `_symbol_${_sanitizeIdentifier(ref.toString())}_${symbolIndex++}`;


and symbolId

DzmitryShylovich · 2017-01-03T19:34:27Z

@vicb done

maxkoretskyi · 2017-02-16T08:41:14Z

Is Symbol essentially a replacement for OpaqueToken ?

DzmitryShylovich · 2017-02-16T08:44:37Z

nope

maxkoretskyi · 2017-02-16T08:59:43Z

It would be interesting to know what OpaqueToken can do that Symbol can't

Closes angular#13314

angular#13701)" This reverts commit 8b5c6b2. This feature is not compatible with the `Injector.get` which now only takes `Type` or `InjectableToken`. `Symbol` is not a valid type. Closes angular#15183

#13701)" (#15319) This reverts commit 8b5c6b2. This feature is not compatible with the `Injector.get` which now only takes `Type` or `InjectableToken`. `Symbol` is not a valid type. Closes #15183 PR Close #15319

…ar#13701) Closes angular#13314

angular#13701)" (angular#15319) This reverts commit 8b5c6b2. This feature is not compatible with the `Injector.get` which now only takes `Type` or `InjectableToken`. `Symbol` is not a valid type. Closes angular#15183 PR Close angular#15319

…ar#13701) Closes angular#13314

angular#13701)" (angular#15319) This reverts commit 8b5c6b2. This feature is not compatible with the `Injector.get` which now only takes `Type` or `InjectableToken`. `Symbol` is not a valid type. Closes angular#15183 PR Close angular#15319

angular-automatic-lock-bot · 2019-09-11T00:21:49Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Dec 29, 2016

vicb reviewed Dec 29, 2016

View reviewed changes

vicb added area: core Issues related to the framework runtime action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 29, 2016

vicb reviewed Dec 29, 2016

View reviewed changes

vicb reviewed Jan 3, 2017

View reviewed changes

vicb added the pr_state: LGTM label Jan 3, 2017

mhevery approved these changes Feb 10, 2017

View reviewed changes

mhevery added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 10, 2017

chuckjaz removed the deprecated label: LGTM (DO NOT USE ANY MORE) label Mar 7, 2017

fix(compiler): shouldn't throw when Symbol is used as DI token

d76fad7

Closes angular#13314

chuckjaz merged commit 8b5c6b2 into angular:master Mar 10, 2017

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

fix(compiler): shouldn't throw when Symbol is used as DI token (angul…

92adbae

…ar#13701) Closes angular#13314

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

fix(compiler): shouldn't throw when Symbol is used as DI token (angul…

c1b5322

…ar#13701) Closes angular#13314

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019

fix(compiler): don't throw when Symbol is used as DI token #13701

fix(compiler): don't throw when Symbol is used as DI token #13701

Uh oh!

Conversation

DzmitryShylovich commented Dec 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vicb Dec 29, 2016

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich Dec 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb Dec 29, 2016

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich commented Dec 29, 2016

Uh oh!

vicb Dec 29, 2016

Choose a reason for hiding this comment

Uh oh!

vicb Dec 29, 2016

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich commented Dec 29, 2016

Uh oh!

vicb Dec 29, 2016

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich commented Dec 30, 2016

Uh oh!

vicb Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb Jan 3, 2017

Choose a reason for hiding this comment

Uh oh!

DzmitryShylovich commented Jan 3, 2017

Uh oh!

maxkoretskyi commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DzmitryShylovich commented Feb 16, 2017

Uh oh!

maxkoretskyi commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

angular-automatic-lock-bot bot commented Sep 11, 2019

Uh oh!

Uh oh!

DzmitryShylovich commented Dec 29, 2016 •

edited

Loading

DzmitryShylovich Dec 29, 2016 •

edited

Loading

vicb Jan 3, 2017 •

edited

Loading

maxkoretskyi commented Feb 16, 2017 •

edited

Loading

maxkoretskyi commented Feb 16, 2017 •

edited

Loading