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

fix PreferenceChangeEvent<T> type #9057

Merged
merged 1 commit into from
Feb 23, 2021

Conversation

paul-marechal
Copy link
Member

What it does

Improves the PreferenceChangeEvent<T> type to properly reflect how it is meant to be used. I also changed the few places where the type was given wrong information. See commit message for details.

How to test

You can inspect the type of event.newValue in places where we check event.preferenceName and make sure that instead of inferring an union of all values for all preference names, we now have the proper type.

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added enhancement issues that are enhancements to current functionality - nice to haves preferences issues related to preferences core issues related to the core of the application labels Feb 12, 2021
@paul-marechal
Copy link
Member Author

I noticed this oddity while reviewing something else, figured we could fix it.

@paul-marechal paul-marechal force-pushed the mp/preference-change-event-type-fix branch 2 times, most recently from 42bee66 to de72ff2 Compare February 12, 2021 01:59
Despite precisely knowing for a given `preferenceName` what its
associated type for `newValue` and `oldValue` should be, the current
typing wasn't allowing TypeScript to properly infer the type.

With this commit, we can do things like:

    declare const event: PreferenceChangeEvent<{
        a: string
	b: number
    }>
    if (event.preferenceName === 'a') {
        event.newValue // type is `string`
    }

The type is more complex, but it now represents how it is meant to be
used rather than getting tsserver lost like before.

This commit also fixes places where the typing was broken and makes use
of the new information.

Signed-off-by: Paul Maréchal <[email protected]>
@paul-marechal paul-marechal force-pushed the mp/preference-change-event-type-fix branch from de72ff2 to a42b7a6 Compare February 12, 2021 16:18
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me, and fix the issue I've noticed when dealing with the onPreferenceChanged event among other things.

We now have the correct typing when doing the following and use the event directly:

this.preferences.onPreferenceChanged(event =>  {
    if (event.preferenceName === 'git.editor.dirtyDiff.linesLimit') {
        const value = event.newValue; // we correctly have a number.
    }
});

rather than the workaround for master:

this.preferences.onPreferenceChanged(event =>  {
    if (event.preferenceName === 'git.editor.dirtyDiff.linesLimit') {
        const newValue = event.newValue; // newValue does not return the proper typings (returns all typings from the schema).
        const value = this.preferences['git.editor.dirtyDiff.linesLimit']; // workaround to get the proper newValue.
    }
});

@paul-marechal paul-marechal merged commit e84bb9e into master Feb 23, 2021
@paul-marechal paul-marechal deleted the mp/preference-change-event-type-fix branch February 23, 2021 15:32
@github-actions github-actions bot added this to the 1.11.0 milestone Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the core of the application enhancement issues that are enhancements to current functionality - nice to haves preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants