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
Assignees
Labels
lang:typescript Issues affecting TypeScript-specific constructs (not general JS issues) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:bug Issues identifying ugly output, or a defect in the program
Milestone

Comments

@JounQin
Copy link
Member

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
Copy link
Member

Regression from #5799.

cc @JamesHenry

@ikatyang ikatyang added type:bug Issues identifying ugly output, or a defect in the program priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! lang:typescript Issues affecting TypeScript-specific constructs (not general JS issues) labels Jan 30, 2019
@ikatyang ikatyang added this to the 1.16.3 milestone Jan 30, 2019
@ikatyang
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Member

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 added the status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! label Feb 2, 2019
@Jessidhia
Copy link

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
Copy link
Collaborator

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
Copy link
Member

j-f1 commented Feb 2, 2019

Should I close this PR then?

@JamesHenry
Copy link
Collaborator

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

@j-f1
Copy link
Member

j-f1 commented Feb 2, 2019

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

@JamesHenry
Copy link
Collaborator

All good 👍

@armano2
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Member

j-f1 commented Feb 5, 2019

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

brodybits pushed a commit to bangkokjs/prettierx-0.4.x-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 pushed a commit to bangkokjs/prettierx-0.4.x-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
Copy link
Contributor

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

@alexander-akait
Copy link
Member

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

brodybits pushed a commit to bangkokjs/prettierx-0.4.x-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
Copy link
Contributor

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 pushed a commit to bangkokjs/prettierx-0.4.x-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
10 tasks
@duailibe duailibe closed this as completed Apr 8, 2019
@ikatyang ikatyang modified the milestones: 1.17, 1.16.3 Apr 12, 2019
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 11, 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.
Labels
lang:typescript Issues affecting TypeScript-specific constructs (not general JS issues) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:bug Issues identifying ugly output, or a defect in the program
Projects
None yet
9 participants