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 check for delete expression must be optional #37921

Merged
merged 2 commits into from
Apr 22, 2020

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Apr 13, 2020

Fixes #13783

@j-oliveras
Copy link
Contributor

Is needed a test with an index type? Like:

interface Bar {
    [id: string]: string;
}

declare const a: Bar

delete a["a"];
delete a.b;

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 13, 2020

Is needed a test with an index type? Like:

Good point.

@typescript-bot pack this.

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 13, 2020

Hey, How are you? @typescript-bot

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 13, 2020

Heya @Kingwl, I've started to run the tarball bundle task on this PR at 6e4b381. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 13, 2020

Hey @Kingwl, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/71131/artifacts?artifactName=tgz&fileId=48169EB321B4086E6AE66D6A286AB7E7D6AA2CCF9BF40D7869FAD1ED36C17C9602&fileName=/typescript-3.9.0-insiders.20200413.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@sandersn sandersn added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 21, 2020
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Apr 24, 2020

This is a breaking change that was added in post beta with no test run on it. I think we should have had more due diligence before pulling it in. I think we should back it out and add it in for 4.0.

@simhnna
Copy link

simhnna commented Sep 17, 2020

What's the reasoning behind excluding null from this list?

the operand must now be any, unknown, never, or be optional (in that it contains undefined in the type)

@DanielRosenwasser
Copy link
Member

Because delete doesn't leave behind a null.

@rohitf
Copy link

rohitf commented Sep 17, 2020

Seeing a possible bug for the following case - even though TS recognizes that key in delete p[key] can be either 'foo' or 'bar', it does not error.


interface Props {
    foo: string;
    bar: string;
}
const p: Props = {
    foo: 'hello',
    bar: 'world'
}

const keys = ['foo', 'bar'] as const;
keys.forEach(key => delete p[key]) // Should error since foo and bar are required props

Playground Link

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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

deleting a prop doesn't invalidate the interface
9 participants