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

Extending computed class name identifiers throws: #2182

Closed
mohsen1 opened this Issue Dec 4, 2016 · 20 comments

Comments

Projects
None yet
@mohsen1

mohsen1 commented Dec 4, 2016

Input:

var a = {
  foo: class {}
}

class bar extends a['foo']{
  
}

CC throws:

JSC_DYNAMIC_EXTENDS_TYPE: The class in an extends clause must be a qualified name. at line 6 character 18
class bar extends a['foo']{
                  ^

Is this by design? The JS code is valid.

@mohsen1

This comment has been minimized.

Show comment
Hide comment
@mohsen1

mohsen1 Dec 4, 2016

I think CC should not treat bracket property accessor with string value as "dynamic". In my example class bar extends a.foo works but class bar extends a['foo'] doesn't while both are the same.

TypeScript compiler can follow the code:

screen shot 2016-12-04 at 10 40 08 am

mohsen1 commented Dec 4, 2016

I think CC should not treat bracket property accessor with string value as "dynamic". In my example class bar extends a.foo works but class bar extends a['foo'] doesn't while both are the same.

TypeScript compiler can follow the code:

screen shot 2016-12-04 at 10 40 08 am

@MatrixFrog

This comment has been minimized.

Show comment
Hide comment
@MatrixFrog

MatrixFrog Dec 4, 2016

Contributor
Contributor

MatrixFrog commented Dec 4, 2016

@mohsen1

This comment has been minimized.

Show comment
Hide comment
@mohsen1

mohsen1 Dec 4, 2016

Webpack outputs code like this:

class APIClientError extends __WEBPACK_IMPORTED_MODULE_3_helpers__["e" /* CustomError */] {

And I am using CC to minify Webpack output.

mohsen1 commented Dec 4, 2016

Webpack outputs code like this:

class APIClientError extends __WEBPACK_IMPORTED_MODULE_3_helpers__["e" /* CustomError */] {

And I am using CC to minify Webpack output.

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle Jan 24, 2017

Contributor

Duplicate of #2106 ?

Contributor

alexeagle commented Jan 24, 2017

Duplicate of #2106 ?

@ChadKillingsworth

This comment has been minimized.

Show comment
Hide comment
@ChadKillingsworth

ChadKillingsworth Jan 24, 2017

Collaborator

Not a duplicate - it's different. Fixing this would be a pretty simple change to the transpilation. Should be non-controversial as well.

Collaborator

ChadKillingsworth commented Jan 24, 2017

Not a duplicate - it's different. Fixing this would be a pretty simple change to the transpilation. Should be non-controversial as well.

@tomccabe

This comment has been minimized.

Show comment
Hide comment
@tomccabe

tomccabe Feb 7, 2017

Just ran into this as well trying to replace the ever-bloating Babel with CC. Sadly I'm getting otherwise great results, but this error pretty much prevents us from adopting CC.

tomccabe commented Feb 7, 2017

Just ran into this as well trying to replace the ever-bloating Babel with CC. Sadly I'm getting otherwise great results, but this error pretty much prevents us from adopting CC.

@dimvar dimvar added the P2 label Feb 7, 2017

@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Feb 23, 2017

Webpack outputs code like this:

class APIClientError extends __WEBPACK_IMPORTED_MODULE_3_helpers__["e" /* CustomError */] {

@mohsen1 webpack would only replace that if you are extending a namespace / import / etc. that you have imported. Can you please share the original source?

TheLarkInn commented Feb 23, 2017

Webpack outputs code like this:

class APIClientError extends __WEBPACK_IMPORTED_MODULE_3_helpers__["e" /* CustomError */] {

@mohsen1 webpack would only replace that if you are extending a namespace / import / etc. that you have imported. Can you please share the original source?

@mohsen1

This comment has been minimized.

Show comment
Hide comment
@mohsen1

mohsen1 Feb 23, 2017

I don't exactly remember the original code anymore but this is close enough:

// Errors.ts
export class CustomError {
// ....
}
// SomeFile.ts
import { CustomError } from './Errors.ts';

class APIClientError extends CustomError {
// ...
}

mohsen1 commented Feb 23, 2017

I don't exactly remember the original code anymore but this is close enough:

// Errors.ts
export class CustomError {
// ....
}
// SomeFile.ts
import { CustomError } from './Errors.ts';

class APIClientError extends CustomError {
// ...
}
@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Mar 20, 2017

As workaround:

// SomeFile.ts
import { CustomError } from './Errors.ts';

const CustomErrorTemp = CustomError;

class APIClientError extends CustomErrorTemp {
// ...
}

sokra commented Mar 20, 2017

As workaround:

// SomeFile.ts
import { CustomError } from './Errors.ts';

const CustomErrorTemp = CustomError;

class APIClientError extends CustomErrorTemp {
// ...
}
@vanhoavn

This comment has been minimized.

Show comment
Hide comment
@vanhoavn

vanhoavn Mar 21, 2017

The problem is from cases like this from typescript in ES6 mode:

class APIClientError extends CustomError<API> {
// ...
}

we cant do:

const CustomErrorTemp = CustomError<API>;

vanhoavn commented Mar 21, 2017

The problem is from cases like this from typescript in ES6 mode:

class APIClientError extends CustomError<API> {
// ...
}

we cant do:

const CustomErrorTemp = CustomError<API>;
@mohsen1

This comment has been minimized.

Show comment
Hide comment
@mohsen1

mohsen1 Mar 21, 2017

You should be able to do that with type:

type CustomErrorTemp = CustomError<API>;

class APIClientError extends CustomErrorTemp {
// ...
}

mohsen1 commented Mar 21, 2017

You should be able to do that with type:

type CustomErrorTemp = CustomError<API>;

class APIClientError extends CustomErrorTemp {
// ...
}
@vanhoavn

This comment has been minimized.

Show comment
Hide comment
@vanhoavn

vanhoavn Mar 21, 2017

Hmm error:

error TS2693: 'CustomErrorTemp' only refers to a type, but is being used as a value here.

vanhoavn commented Mar 21, 2017

Hmm error:

error TS2693: 'CustomErrorTemp' only refers to a type, but is being used as a value here.
@mohsen1

This comment has been minimized.

Show comment
Hide comment
@mohsen1

mohsen1 Mar 21, 2017

oh right... sorry for misleading comment

mohsen1 commented Mar 21, 2017

oh right... sorry for misleading comment

@Treeki

This comment has been minimized.

Show comment
Hide comment
@Treeki

Treeki Oct 25, 2017

I ran into this issue and decided to take a stab at fixing it (despite never having touched the Closure Compiler codebase before...). As far as I can tell, the most elegant solution is to allow CC to consider terms like a.b['c'] as a qualified name, similar (but not equivalent!) to a.b.c.

I don't want to submit a PR for this as my fix isn't perfect -- it fails some tests -- but perhaps somebody more familiar with the codebase can build upon it and complete it: https://github.com/Treeki/closure-compiler/tree/allow-constant-getelem-in-qualified-names

Treeki commented Oct 25, 2017

I ran into this issue and decided to take a stab at fixing it (despite never having touched the Closure Compiler codebase before...). As far as I can tell, the most elegant solution is to allow CC to consider terms like a.b['c'] as a qualified name, similar (but not equivalent!) to a.b.c.

I don't want to submit a PR for this as my fix isn't perfect -- it fails some tests -- but perhaps somebody more familiar with the codebase can build upon it and complete it: https://github.com/Treeki/closure-compiler/tree/allow-constant-getelem-in-qualified-names

aomarks added a commit to aomarks/tsickle that referenced this issue Feb 6, 2018

Fix crash when class declaration extends an expression.
Fixes angular#679.

It's possible for a class declaration to extend an expression that does
not have a symbol, for example when a mixin function is used to build a
base class, as in `declare MyClass extends MyMixin(MyBaseClass)`.

Handling this correctly is tricky. Closure throws on this
`extends <expression>` syntax (see
google/closure-compiler#2182). We would
probably need to generate an intermediate class declaration and extend
that. For now, just omit the `extends` annotation.

mprobst added a commit to angular/tsickle that referenced this issue Feb 6, 2018

Fix crash when class declaration extends an expression.
Fixes #679.

It's possible for a class declaration to extend an expression that does
not have a symbol, for example when a mixin function is used to build a
base class, as in `declare MyClass extends MyMixin(MyBaseClass)`.

Handling this correctly is tricky. Closure throws on this
`extends <expression>` syntax (see
google/closure-compiler#2182). We would
probably need to generate an intermediate class declaration and extend
that. For now, just omit the `extends` annotation.

ncfxy added a commit to ncfxy/tsickle that referenced this issue Apr 24, 2018

Fix crash when class declaration extends an expression.
Fixes angular#679.

It's possible for a class declaration to extend an expression that does
not have a symbol, for example when a mixin function is used to build a
base class, as in `declare MyClass extends MyMixin(MyBaseClass)`.

Handling this correctly is tricky. Closure throws on this
`extends <expression>` syntax (see
google/closure-compiler#2182). We would
probably need to generate an intermediate class declaration and extend
that. For now, just omit the `extends` annotation.
@nichtverstehen

This comment has been minimized.

Show comment
Hide comment
@nichtverstehen

nichtverstehen May 17, 2018

I deal with this limitation by manually extracting the base into a variable. I wonder if applying such a transform mechanically could be a correct resolution for this issue.

I.e. for the code in the first comment, the rewriting could be:

var a = {
  foo: class {}
}

var barBase0 = a['foo'];
class bar extends barBase0 {
  // ...
}

Will this be typed as expected?

nichtverstehen commented May 17, 2018

I deal with this limitation by manually extracting the base into a variable. I wonder if applying such a transform mechanically could be a correct resolution for this issue.

I.e. for the code in the first comment, the rewriting could be:

var a = {
  foo: class {}
}

var barBase0 = a['foo'];
class bar extends barBase0 {
  // ...
}

Will this be typed as expected?

@nichtverstehen

This comment has been minimized.

Show comment
Hide comment
@nichtverstehen

nichtverstehen May 17, 2018

Hmm, I see I unassigned MatrixFrog here, but I didn't intend to; I was just posting a comment. I don't think I even have a permission to assign or unassign anyone here. Please disregard and re-assign the issue back if you are an owner.

nichtverstehen commented May 17, 2018

Hmm, I see I unassigned MatrixFrog here, but I didn't intend to; I was just posting a comment. I don't think I even have a permission to assign or unassign anyone here. Please disregard and re-assign the issue back if you are an owner.

@paulfisher53

This comment has been minimized.

Show comment
Hide comment
@paulfisher53

paulfisher53 Jul 3, 2018

I'm also experiencing this issue with webpack + closure compiler. Is there any ETA on a fix?

paulfisher53 commented Jul 3, 2018

I'm also experiencing this issue with webpack + closure compiler. Is there any ETA on a fix?

@ChadKillingsworth

This comment has been minimized.

Show comment
Hide comment
@ChadKillingsworth
Collaborator

ChadKillingsworth commented Jul 3, 2018

See #2997

@ChadKillingsworth

This comment has been minimized.

Show comment
Hide comment
@ChadKillingsworth

ChadKillingsworth Aug 10, 2018

Collaborator

Closed by #2997

Collaborator

ChadKillingsworth commented Aug 10, 2018

Closed by #2997

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment