Skip to content

Do not emit code for @extends tags in JS. #29244

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

Merged
merged 1 commit into from
Jan 4, 2019

Conversation

mprobst
Copy link
Contributor

@mprobst mprobst commented Jan 3, 2019

When transpiling JavaScript, TS3.1+ emits @extends tags as code. E.g.

/** @extends {SuperClass} */
class SubClass {}

Causes an ES5 emit that references SuperClass:

/**
* @extends {SomeBase}
*/
var SubClass = /** @class */ (function (_super) {
    __extends(SubClass, _super);
    function SubClass() {
        return _super !== null && _super.apply(this, arguments) || this;
    }
    return SubClass;
}(SomeBase));

Note the literal references to SomeBase.

This appears to be an accidental effect of
0f55566. It refactored
getEffectiveBaseTypeNode for type checking, but missed an instance
where it is also used for emit logic. This change fixes the problem by
specifically getting the heritage clauses directly off the AST.

@mprobst
Copy link
Contributor Author

mprobst commented Jan 3, 2019

/CC @sandersn

@mprobst mprobst force-pushed the extend-js branch 2 times, most recently from 80cca24 to fd7a197 Compare January 3, 2019 14:12
@weswigham
Copy link
Member

weswigham commented Jan 3, 2019

@mprobst you'll find the tests easier to write/baseline if you add a compiler test instead of a project test (I don't think we ever really add new project tests much anymore). Something like

// @outDir: ./out
// @allowJs: true
// @filename: extends.js
/** @extends {SuperClass} */
class SubClass {}

should work.

@mprobst
Copy link
Contributor Author

mprobst commented Jan 3, 2019

@weswigham thanks! I'll take a look at that tomorrow.

I don't understand my test failure, locally npm test passes. Maybe a Windows vs Linux issue? Any idea?

@weswigham
Copy link
Member

I would guess it's failing because your input file, tests/cases/projects/jsFileCompilation/ExtendsTag/extends.js, isn't actually in the PR 😄

@weswigham
Copy link
Member

You definitely want to write a compiler test instead, however 😉

When transpiling JavaScript, TS3.1+ emits `@extends` tags as code. E.g.

    /** @extends {SuperClass} */
    class SubClass {}

Causes an ES5 emit that references SuperClass:

    /**
    * @extends {SomeBase}
    */
    var SubClass = /** @Class */ (function (_super) {
        __extends(SubClass, _super);
        function SubClass() {
            return _super !== null && _super.apply(this, arguments) || this;
        }
        return SubClass;
    }(SomeBase));

Note the literal references to `SomeBase`.

This appears to be an accidental effect of 0f55566. It refactored
`getEffectiveBaseTypeNode` for type checking, but missed an instance
where it is also used for emit logic. This change fixes the problem by
specifically getting the heritage clauses directly off the AST.

Change-Id: I3128a757e5924e2528c61230a90ac13650852542
@mprobst
Copy link
Contributor Author

mprobst commented Jan 4, 2019

💡 .gitignore caused the file to be ignored, silently.

For context, I was looking for tests that used *.js and only found the projects cases, didn't realize @filename was the thing to look for.

I've moved to a compiler test case, that's indeed much simpler.

@weswigham weswigham merged commit 7a2b2ce into microsoft:master Jan 4, 2019
@mprobst mprobst deleted the extend-js branch January 4, 2019 16:55
ajafff added a commit to ajafff/TypeScript that referenced this pull request Jan 8, 2019
This fixes a similar problem as microsoft#29244 where JSDoc `@extends`
sandersn pushed a commit that referenced this pull request Jan 9, 2019
* Exclude JSDoc @extends from 'super()' checks

This fixes a similar problem as #29244 where JSDoc `@extends`

* fix check 'super can only be referenced in a derived class'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants