Skip to content

Fix: convertFunctionToEs6Class cannot recognize x.prototype = {} pattern #35219

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

Merged
merged 5 commits into from
Apr 30, 2020

Conversation

Jack-Works
Copy link
Contributor

Currently, TypeScript's fixer cannot recognize this pattern:

function X() {}
X.prototype = {
    method(): {}
}

This PR add the ability to recognize this pattern.

Before

image

After

image

@Jack-Works Jack-Works marked this pull request as ready for review November 21, 2019 03:45
@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 3, 2020
@sandersn sandersn assigned andrewbranch and unassigned minestarks Mar 10, 2020
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! I think it looks good, but I don’t think we can merge it without some accompanying tests. (If we make changes here in the future, we need to know what this does and whether our new changes are breaking it. Uncovered code tends to get broken or deleted eventually.)

@andrewbranch
Copy link
Member

Also, I just noticed how old this PR is, and I’m sorry for the delay—I know it’s not super cool to leave this sitting for 5 months and then request changes. We’re finally making a real effort to get our PR backlog organized, so hopefully we’ll be faster to respond in the future. Thanks for understanding ❤️

@Jack-Works
Copy link
Contributor Author

cool, I'll add some test later

@Jack-Works Jack-Works force-pushed the fix/convertToES6Class branch from 08ffbd3 to e478808 Compare April 1, 2020 23:19
@ExE-Boss
Copy link
Contributor

ExE-Boss commented Apr 2, 2020

What about:

function Bar(...args) {
	// code
}

Bar.prototype = {
	// Should be ignored, as classes already include this:
	constructor: Bar,

	// Should result in `someField` being added to the class as a field.
	someField: 123,
};

@Jack-Works
Copy link
Contributor Author

What about:

function Bar(...args) {
	// code
}

Bar.prototype = {
	// Should be ignored, as classes already include this:
	constructor: Bar,

	// Should result in `someField` being added to the class as a field.
	someField: 123,
};

The removal of constructor is done. But the class field is not implemented yet. (And I'm doubting the runtime behavior of prototype and class field are different.)

@Jack-Works
Copy link
Contributor Author

@andrewbranch hi I need help. the tests passed on my machine but not on CI

@andrewbranch
Copy link
Member

@Jack-Works unrelated to the failures, but can you move your test files out from the server subdirectory of fourslash? (That folder runs with additional mocking infrastructure that shouldn’t be relevant to your tests.) Secondly, it looks like the failures are due to a newline character mismatch in your expected output. Maybe git rewrote line endings in your commit. Try to normalize all the line endings in your test files, and ensure whatever you pick doesn’t get overridden by git or your editor on save. My guess is that some process tried to normalize line endings for you, but refused to change the contents of a template string. So, the actual content (the part with the four slashes) changed, but the expected content in the template string did not.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for iterating on the original code; this is much easier to read now 🌟

@Jack-Works
Copy link
Contributor Author

hi @andrewbranch when to merge?

@andrewbranch
Copy link
Member

Thanks for the reminder, we were waiting until master reflected 4.0.

@andrewbranch andrewbranch merged commit 6f7faa7 into microsoft:master Apr 30, 2020
@Jack-Works Jack-Works deleted the fix/convertToES6Class branch May 1, 2020 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants