Skip to content

Conversation

jakebailey
Copy link
Member

Fixes #48915

This is probably breaking, so for 4.9 if we want it.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jun 29, 2022
>this.c = 'nested object' : "nested object"
>this.c : any
>this : this
>this : { C: typeof C; }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is explicitly the case that is changing:

var o = {
    C: function () {
        this.c = 'nested object'
    }
}
var og = new o.C();

So, I'm not sure if that's good or bad.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can see in the existing code that this probably already works, there doesn’t seem to be a test (at least in ones you added or changed) that uses /** @class */ inside an object.

Come to think of it, what should the behavior of

const o = {
  SomeClass() {
    this.x = 0;
  }
}

be (with and without @class)? I don’t know, but let’s add it to tests too so we have a documented position.

@jakebailey
Copy link
Member Author

I think that https://github.com/microsoft/TypeScript/pull/49735/files#diff-dbf882263a483218df32c53af90df429250234fb325ba71f8dd34bcbe165fe54 is that case, but you're right that I forgot one that's tagged (I don't know if there is one; I would assume so but I will check).

@jakebailey
Copy link
Member Author

I missed that you had a method declaration there and not a function expression. The runtime behavior is the same, so I'm inclined to make it the same. I'll add an explicit test.

@jakebailey jakebailey requested a review from sandersn June 30, 2022 04:27
@jakebailey
Copy link
Member Author

Handled that case. I'm a little bit unsure what the behavior should be for some of those things that changed. #27561 is relevant.

@andrewbranch
Copy link
Member

I’m not sure I think it’s very important to make object literal method declarations classable. That code looks bananas to me.

@jakebailey jakebailey requested a review from andrewbranch July 6, 2022 16:16
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I think we decided to wait until 4.9 for this, right?

@andrewbranch andrewbranch added the Breaking Change Would introduce errors in existing code label Jul 6, 2022
@jakebailey
Copy link
Member Author

In #48915 (comment), I think so, yes. I don't know if this is urgent enough to go into 4.8.

@andrewbranch
Copy link
Member

I think the breakiness is fairly low, but the bug is not a recent regression, has no upvotes, and can be worked around, so for me it doesn’t meet the bar for considering it now.

@jakebailey jakebailey added this to the TypeScript 4.9.0 milestone Jul 6, 2022
@jakebailey jakebailey merged commit 5fbf3b0 into microsoft:main Aug 10, 2022
@jakebailey jakebailey deleted the fix-48915 branch August 10, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change Would introduce errors in existing code For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

IntelliSense/autocomplete doesn't work properly inside object literals
4 participants