Skip to content
This repository was archived by the owner on Jan 19, 2019. It is now read-only.

New: Support Definite Assignment (fixes #424) #432

Merged
JamesHenry merged 1 commit into from
Feb 21, 2018
Merged

New: Support Definite Assignment (fixes #424) #432

JamesHenry merged 1 commit into from
Feb 21, 2018

Conversation

@azz
Copy link
Contributor

@azz azz commented Jan 11, 2018 •
edited
Loading

Build will fail due to this not yet supported in Babylon.

@jsf-clabot
Copy link

jsf-clabot commented Jan 11, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@azz azz force-pushed the definite-assignment branch from 0e86d65 to cf1b7ef Compare January 11, 2018 05:43
@azz azz changed the title New: Support Definite Assignment (Fixes #424) New: Support Definite Assignment (fixes #424) Jan 11, 2018
JamesHenry
JamesHenry suggested changes Jan 12, 2018
View reviewed changes
Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

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

Thanks @azz!

The name of this property is still a little TBD.

It looks like it might end up as a something like definteAssignment, but I think it is very unlikely that exclamationToken will be used.

You can see the discussion with the TS Team here: babel/babel#7159

Like you say we will need to to wait until babylon supports it

@azz
Copy link
Contributor Author

azz commented Jan 12, 2018

Did you catch this comment? #424 (comment)

@JamesHenry
Copy link
Member

JamesHenry commented Jan 12, 2018 •
edited
Loading

Added my 2 cents :) (or +1ing of someone else's ha)

@azz
Copy link
Contributor Author

azz commented Jan 12, 2018

I have no strong opinions either way 🤷‍♂️

@JamesHenry
Copy link
Member

JamesHenry commented Jan 12, 2018

As with the unique symbol one, I've started a protected ts-2.7 branch as we normally do with changes which relate to not yet stable versions of TS. Please change the base to be that branch

@JamesHenry
Copy link
Member

JamesHenry commented Jan 12, 2018 •
edited
Loading

I'm the same regarding opinion of the change - but I am strongly of the opinion that we choose whatever ends up being used in that babylon PR so that we stay aligned 👍

@azz azz changed the base branch from master to ts-2.7 January 12, 2018 15:15
@j-f1 j-f1 mentioned this pull request Jan 31, 2018
Closed
@JamesHenry
Copy link
Member

JamesHenry commented Feb 9, 2018

Again, with this one @azz I am just going to merge this into the 2.7 branch and then sort out any issues/cleanup. Thanks again!

@JamesHenry
Copy link
Member

JamesHenry commented Feb 9, 2018

Ah no, I can't sorry, cause of the conflicts. There is also yarn.lock file that needs to be removed.

@JamesHenry
Copy link
Member

JamesHenry commented Feb 9, 2018

I've pressed ahead with the TS 2.7 release so that's all merged into master now.

@azz azz force-pushed the definite-assignment branch from cf1b7ef to 02f1e48 Compare February 10, 2018 04:12
@azz azz changed the base branch from ts-2.7 to master February 10, 2018 04:18
@azz
Copy link
Contributor Author

azz commented Feb 10, 2018

Changed base branch and fixed conflicts.

@azz
Copy link
Contributor Author

azz commented Feb 17, 2018

Ping @JamesHenry, we'd really like to get this out to Prettier users!

j-f1
j-f1 reviewed Feb 17, 2018
View reviewed changes
lib/convert.js Outdated
}

if (node.exclamationToken) {
result.exclamationToken = true;
Copy link
Contributor

@j-f1 j-f1 Feb 17, 2018

Choose a reason for hiding this comment

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

I suggested definite in the Babylon PR, what do you think?

Copy link
Contributor Author

@azz azz Feb 17, 2018

Choose a reason for hiding this comment

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

We'll follow babylon's choice - updated to definite.

Copy link
Member

@JamesHenry JamesHenry Feb 17, 2018

Choose a reason for hiding this comment

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

Thanks 👍

@azz azz force-pushed the definite-assignment branch from 02f1e48 to 2cbd605 Compare February 17, 2018 13:57
@JamesHenry
Copy link
Member

JamesHenry commented Feb 17, 2018

The build isn't passing, please just add your new specs to the ignore patterns within ast-alignment testing for now: https://github.com/eslint/typescript-eslint-parser/blob/master/tests/ast-alignment/fixtures-to-test.js#L341

@azz azz force-pushed the definite-assignment branch from 2cbd605 to 4a9d443 Compare February 18, 2018 00:56
@azz
Copy link
Contributor Author

azz commented Feb 18, 2018

Ah, sorry! Updated.

@JamesHenry JamesHenry merged commit 439ea24 into eslint:master Feb 21, 2018
@JamesHenry
Copy link
Member

JamesHenry commented Feb 21, 2018

Thanks, @azz!

@JamesHenry
Copy link
Member

JamesHenry commented Feb 21, 2018

Released in 14.0.0

@azz azz deleted the definite-assignment branch February 21, 2018 22:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@j-f1 j-f1 j-f1 left review comments

@JamesHenry JamesHenry JamesHenry approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@azz @jsf-clabot @JamesHenry @j-f1