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

Add a test for emit of a computed property based on an aliased symbol #47978

Merged
merged 1 commit into from
Mar 5, 2022

Conversation

Andarist
Copy link
Contributor

This doesn't fix anything - it's just a test for something that I think is not explicitly tested anywhere.

I've noticed that this has started to work in 4.3.0:

So I've dug a little bit deeper and I've found out that this has started to work in 4.3.0-dev.20210223 and based on dates of that and the previous nightly release I've concluded that this has changed somewhere here. Looking through those commits I think that most likely this has been enabled by #42543

This PR didn't introduce any PR resembling this usage nor I've found any in the current codebase so I've just figured out that it would be great to have a test covering for this since I intend to rely on this behavior 😅

cc @weswigham

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Feb 20, 2022
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Copy link

@meruyert93 meruyert93 left a comment

Choose a reason for hiding this comment

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

I think test coverage is indeed great idea and it looks clean!

@weswigham
Copy link
Member

What's the part you think we may not have a test of already?

@Andarist
Copy link
Contributor Author

I've looked for one 😅 specifically for a case where a variable with such type would be used as a computed property. All tests seem to only use [Symbol.whatever]. I could miss something but i've grepped the test suite, looked through your PR that has made this possible and looked manually through all current tests in the Symbol "category"

@sandersn sandersn requested a review from weswigham February 28, 2022 18:41
@sandersn sandersn added the Housekeeping Housekeeping PRs label Feb 28, 2022
@weswigham
Copy link
Member

That would be because they're all generic unique symbol tests because my PR essentially just made the SymbolConstructor members into unique symbols (even if not declared as such). And we definitely have unique symbol computed property name tests.

@Andarist
Copy link
Contributor Author

Andarist commented Mar 1, 2022

@weswigham I've taken another round throughout the test suite and I can't find a test like this. Knowing the implementation details perhaps this is just the same as:

export class MyObservable<T> {
    constructor(private _val: T) {}

    subscribe(next: (val: T) => void) {
        next(this._val)
    }

    [Symbol.obs]() {
        return this
    }
}

Here, we also have a computed property. However, this has already worked before #42543 and the test case added in this PR wouldn't work the same before that PR.

The closest test that resembles the situation depicted here is this one:

but this file was last touched in 2018 which predates #42543. So since that PR fixed the problem from this added test here, my intuition says that this similar~ test is actually testing something else.

The thing that makes this test here somewhat special is not the computed property itself. It's that this symbol type is first assigned to a variable and the computed property is declared using it.

@weswigham weswigham merged commit c1783b2 into microsoft:main Mar 5, 2022
@Andarist Andarist deleted the test/aliased-symbol-emit branch March 6, 2022 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug Housekeeping Housekeeping PRs
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants