Skip to content

Handle imports and exports in 'convert parameters to destructured object' #30475

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 36 commits into from
Mar 28, 2019

Conversation

gabritto
Copy link
Member

Contributes to #30113.
This PR allows the 'convert parameters to destructured object' to apply changes when the function being refactored is exported or imported with TS-recognized syntax. Previously the refactor would not apply any changes when it found an import or export reference to the function being refactored.
In addition, we can now refactor a function declaration that doesn't have a name but is a default export, and a constructor of a class declaration that doesn't have a name but is a default export.

@gabritto gabritto added this to the TypeScript 3.5.0 milestone Mar 18, 2019
@gabritto gabritto requested a review from amcasey March 26, 2019 23:35

// @Filename: f.ts
////function foo(/*a*/a: string, b: string/*b*/) { }
////export = foo;
Copy link
Member

Choose a reason for hiding this comment

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

I think you can also export = function(.... Is that case supported?

Copy link
Member

Choose a reason for hiding this comment

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

("No" is a reasonable answer - I mostly want to know if the case was considered.)


In reply to: 269694751 [](ancestors = 269694751)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not supported, but I didn't considered it, that's a good point. The refactoring is not offered in this case because this is a function expression (or an arrow function) that is in an export assignment and we only offer it if the function expression is assigned to a const variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

i've briefly thought about supporting this case and it would require some changes to be able to call findAllReferences, so i think it's better to consider supporting this on another PR later.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Some minor questions but generally looks good.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Did any old tests have to change? I thought there were a couple of intentionally-failing ones already checked in.

@gabritto
Copy link
Member Author

Did any old tests have to change? I thought there were a couple of intentionally-failing ones already checked in.

There was only one test that I could find, which was checking that the refactoring was not available on a constructor of a default class without a name, and it should be available now. However the test was passing after the changes in this PR because there was an extra slash, so there were five slashes.

@gabritto gabritto merged commit bb5eb02 into master Mar 28, 2019
@gabritto gabritto deleted the namedParametersImportExport branch March 28, 2019 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants