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

TypeScript generics are removed incorrectly #5817

Closed
JounQin opened this issue Jan 30, 2019 · 17 comments · Fixed by typescript-eslint/typescript-eslint#208
Closed

TypeScript generics are removed incorrectly #5817

JounQin opened this issue Jan 30, 2019 · 17 comments · Fixed by typescript-eslint/typescript-eslint#208

Comments

@JounQin
Copy link
Contributor

@JounQin JounQin commented Jan 30, 2019

Prettier 1.16.2
Playground link

--parser typescript

Input:

export default {
  load<K, T>(k: K, t: T) {
  	return {k, t};
  }
}

Output:

export default {
  load(k: K, t: T) {
    return { k, t };
  }
};

Expected behavior:
preserve TypeScript generics

@ikatyang

This comment has been minimized.

Copy link
Member

@ikatyang ikatyang commented Jan 30, 2019

Regression from #5799.

cc @JamesHenry

@ikatyang

This comment has been minimized.

Copy link
Member

@ikatyang ikatyang commented Jan 31, 2019

@JounQin Sorry for the inconvenience, the temporary workaround (#5818) is released in 1.16.3.

Prettier 1.16.3
Playground link

--parser typescript

Input:

export default {
  load<K, T>(k: K, t: T) {
  	return {k, t};
  }
}

Output:

export default {
  load<K, T>(k: K, t: T) {
    return { k, t };
  }
};
@ikatyang ikatyang modified the milestones: 1.16.3, 1.17 Jan 31, 2019
@JamesHenry

This comment has been minimized.

Copy link
Collaborator

@JamesHenry JamesHenry commented Feb 1, 2019

Thanks for pushing out a patch @ikatyang, I'm guessing we were just missing coverage for this in the prettier tests?

I can work on it and restore the updated version of typescript-estree

@ikatyang

This comment has been minimized.

Copy link
Member

@ikatyang ikatyang commented Feb 1, 2019

Yeah, we probably just need to fix this missing case. It's still the updated version (the OP can be reproduced) on master as I only reverted it in the 1.16 branch.

@j-f1 j-f1 self-assigned this Feb 2, 2019
@j-f1

This comment has been minimized.

Copy link
Member

@j-f1 j-f1 commented Feb 2, 2019

This is because the parser puts the typeParamaters on the Property node rather than the FunctionExpression.

@j-f1 j-f1 mentioned this issue Feb 2, 2019
2 of 3 tasks complete
@j-f1 j-f1 added the status:has pr label Feb 2, 2019
@Jessidhia

This comment has been minimized.

Copy link

@Jessidhia Jessidhia commented Feb 2, 2019

That sounds like a parser bug to me; it's not otherwise possible to have any kind of generics on properties 🤔

But if it works...

@JamesHenry

This comment has been minimized.

Copy link
Collaborator

@JamesHenry JamesHenry commented Feb 2, 2019

@j-f1 Did you not see this? #5817 (comment)

I worked on this, just hadn't opened a PR yet.

I found the AST oddity yesterday and @armano2 and I agreed that it shouldn't be the case and we need to diverge from babel there (and report it)

@j-f1

This comment has been minimized.

Copy link
Member

@j-f1 j-f1 commented Feb 2, 2019

Should I close this PR then?

@JamesHenry

This comment has been minimized.

Copy link
Collaborator

@JamesHenry JamesHenry commented Feb 2, 2019

No, it's the same solution, but obviously want to avoid situations where multiple people are doing the same work

@j-f1

This comment has been minimized.

Copy link
Member

@j-f1 j-f1 commented Feb 2, 2019

Yeah, I didn’t see your comment when I was making the PR. Sorry about that.

@JamesHenry

This comment has been minimized.

Copy link
Collaborator

@JamesHenry JamesHenry commented Feb 2, 2019

All good 👍

@armano2

This comment has been minimized.

Copy link

@armano2 armano2 commented Feb 4, 2019

looks like there is few more issues here:

export class {
  constructor<T, X, F>(k: K, t: T) {
  	return {k, t};
  }
}
@JamesHenry

This comment has been minimized.

Copy link
Collaborator

@JamesHenry JamesHenry commented Feb 4, 2019

Oh wow, merging the PR on the other repo caused this issue to be closed. I'm guessing it piggybacked my permissions on both repos? Don't think I've ever seen that before

@JamesHenry JamesHenry reopened this Feb 4, 2019
@j-f1

This comment has been minimized.

Copy link
Member

@j-f1 j-f1 commented Feb 5, 2019

Yeah, that’s one of the features of GitHub that comes in handy every so often.

brodybits added a commit to brodybits-archive/prettier-fork that referenced this issue Mar 19, 2019
resolves the issue with generics in object methods in TypeScript

and passes the test proposed in the following PR: prettier#5824

resolves prettier#5817
brodybits added a commit to brodybits-archive/prettier-fork that referenced this issue Apr 4, 2019
as proposed by @j-f1 in the following PR: prettier#5824

should validate that prettier#5817 does not reappear

Co-authored-by: Jed Fox <git@twopointzero.us>
Co-authored-by: Christopher J. Brody <chris@brody.consulting>
@brodybits

This comment has been minimized.

Copy link
Contributor

@brodybits brodybits commented Apr 4, 2019

I think this issue is now resolved by PR #6027 (TS 3.4 update which includes @typescript-eslint/typescript-estree version 1.6.0).

@evilebottnawi

This comment has been minimized.

Copy link
Member

@evilebottnawi evilebottnawi commented Apr 4, 2019

I think we need merge test before close this to avoid regressions in future

brodybits added a commit to brodybits-archive/prettier-fork that referenced this issue Apr 4, 2019
as proposed by @j-f1 in the following PR: prettier#5824

should validate that issue prettier#5817 does not reappear

closes prettier#5817 as resolved and tested

Co-authored-by: Jed Fox <git@twopointzero.us>
Co-authored-by: Christopher J. Brody <chris@brody.consulting>
@brodybits

This comment has been minimized.

Copy link
Contributor

@brodybits brodybits commented Apr 4, 2019

I think we need merge test before close this to avoid regressions in future

The commit I proposed in PR #5989 should automatically close this issue as resolved and tested.

Thanks @evilebottnawi for your attention:)

brodybits added a commit to brodybits-archive/prettier-fork that referenced this issue Apr 4, 2019
as proposed by @j-f1 in the following PR: prettier#5824

should validate that issue prettier#5817 does not reappear

this change closes prettier#5817 as resolved and tested

this change closes prettier#5824 (PR prettier#5824) as superseded

Co-authored-by: Jed Fox <git@twopointzero.us>
Co-authored-by: Christopher J. Brody <chris@brody.consulting>
@brodybits brodybits mentioned this issue Apr 8, 2019
0 of 10 tasks complete
@duailibe duailibe closed this Apr 8, 2019
@ikatyang ikatyang modified the milestones: 1.17, 1.16.3 Apr 12, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.