- Notifications
You must be signed in to change notification settings - Fork14
Feat#Provide a quick-fix for non-exported imports#72
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:oss-hackathon-37440
Are you sure you want to change the base?
Feat#Provide a quick-fix for non-exported imports#72
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Signed-off-by: Richard Lynch <rlynch79@bloomberg.net>
Signed-off-by: Richard Lynch <rlynch79@bloomberg.net>
Signed-off-by: Richard Lynch <rlynch79@bloomberg.net>
Signed-off-by: Richard Lynch <rlynch79@bloomberg.net>
82a3253 toc68ae3bCompareRichard-Lynch commentedJun 16, 2022
Maybe worth adding (at)DanielRosenwasser and (at)sandersn when this is added to the TS repo |
mkubilayk left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looking very good - some minor comments.
| constnode=localSymbol.valueDeclaration.parent; | ||
| returnchanges.insertExportModifier(sourceFile,node); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need special handling forclass declarations as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
class is captured/handled withisDeclarationStatement but I've added some tests and simplified that logic
| // If there is an existing export | ||
| if( | ||
| namedExportDeclaration?.exportClause&& | ||
| isNamedExports(namedExportDeclaration.exportClause) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we check if this is a type-only export? My understanding is that this would change:
exporttype{existingThing};
to
export{existingThing,newThing};
which may not be desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nice! yes, this is a good test case, adding now
| if( | ||
| isExportDeclaration(statement)&& | ||
| statement.exportClause&& | ||
| isNamedExports(statement.exportClause) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should skip re-export statements here:export { a } from "./something";.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yup nice, added
| factory.updateExportDeclaration( | ||
| /*node*/namedExportDeclaration, | ||
| /*modifiers*/undefined, | ||
| /*isTypeOnly*/false, | ||
| /*exportClause*/factory.updateNamedExports( | ||
| /*node*/namedExportDeclaration.exportClause, | ||
| /*elements*/sortSpecifiers( | ||
| namedExportDeclaration.exportClause.elements.concat( | ||
| factory.createExportSpecifier( | ||
| /*isTypeOnly*/false, | ||
| /*propertyName*/undefined, | ||
| node | ||
| ) | ||
| ) | ||
| ) | ||
| ), | ||
| /*moduleSpecifier*/undefined, | ||
| /*assertClause*/undefined | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| factory.updateExportDeclaration( | |
| /*node*/namedExportDeclaration, | |
| /*modifiers*/undefined, | |
| /*isTypeOnly*/false, | |
| /*exportClause*/factory.updateNamedExports( | |
| /*node*/namedExportDeclaration.exportClause, | |
| /*elements*/sortSpecifiers( | |
| namedExportDeclaration.exportClause.elements.concat( | |
| factory.createExportSpecifier( | |
| /*isTypeOnly*/false, | |
| /*propertyName*/undefined, | |
| node | |
| ) | |
| ) | |
| ) | |
| ), | |
| /*moduleSpecifier*/undefined, | |
| /*assertClause*/undefined | |
| ) | |
| factory.updateExportDeclaration( | |
| namedExportDeclaration, | |
| /*modifiers*/undefined, | |
| /*isTypeOnly*/false, | |
| factory.updateNamedExports( | |
| namedExportDeclaration.exportClause, | |
| sortSpecifiers( | |
| namedExportDeclaration.exportClause.elements.concat( | |
| factory.createExportSpecifier( | |
| /*isTypeOnly*/false, | |
| /*propertyName*/undefined, | |
| node | |
| ) | |
| ) | |
| ) | |
| ), | |
| /*moduleSpecifier*/undefined, | |
| /*assertClause*/undefined | |
| ) | |
I think some of these arguments are self explanatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm happy to remove any of these you'd like, but tbh as someone new to the project => they're not :P
Honestly half the pain is having so many positional arguments at all!
| /*exportClause*/factory.createNamedExports([ | ||
| factory.createExportSpecifier( | ||
| /*isTypeOnly*/false, | ||
| /*propertyName*/undefined, | ||
| node | ||
| ), | ||
| ]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| /*exportClause*/factory.createNamedExports([ | |
| factory.createExportSpecifier( | |
| /*isTypeOnly*/false, | |
| /*propertyName*/undefined, | |
| node | |
| ), | |
| ]), | |
| factory.createNamedExports([ | |
| factory.createExportSpecifier( | |
| /*isTypeOnly*/false, | |
| /*propertyName*/undefined, | |
| node | |
| ), | |
| ]), | |
| /*expression*/!(isVariableDeclarationList(node)&&node.declarations.length!==1), | ||
| /*message*/"Only allow adding export modifier to variable lists with 1 element" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| /*expression*/!(isVariableDeclarationList(node)&&node.declarations.length!==1), | |
| /*message*/"Only allow adding export modifier to variable lists with 1 element" | |
| !(isVariableDeclarationList(node)&&node.declarations.length!==1), | |
| "Only allow adding export modifier to variable lists with 1 element" | |
| ////let d = 4; | ||
| ////export function whatever() { | ||
| ////} | ||
| ////export { d } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would be good to add some test cases where source file has some exports that we cannot use:
exporttype{a};export{b}from"./b";export*ascfrom"./c";// ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| "code":90058 | ||
| }, | ||
| "Export '{0}' from module '{1}'": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| "Export'{0}' from module'{1}'": { | |
| "Export\"{0}\" from module\"{1}\"": { |
minor, seems to match the other ones above this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yup nice, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
hmm, theRemove import from './b' uses single quotes, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it looks like there are only 4 instances using double quotes, that seems like a mistake
src/compiler/diagnosticMessages.json Outdated
| "category":"Message", | ||
| "code":90059 | ||
| }, | ||
| "Add all missing exports": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I realise this message is simialr to the "Add all imports" one, but I don't think it really expresses the change that will happen. Maybe "Export all missing members in modules"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed, I changed toExport all missing members from modules
| //@Filename: /b.ts | ||
| ////declare function foo(): any; | ||
| ////function bar(): any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would like to see some tests on overloaded functions as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good shout! do you think the export should be on the implemented function or in a named export? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, I've given this a look and its actually a little tricky given the current implementation!
I can easily make all functions export using a named export =>export {add};, but if we want to follow the convention of applying the export on the declaration where possible it'll take a bit more work! Do you think that's critical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think a separate export statement (e.g.export { add }) would be acceptable for function overloads.
| index:1, | ||
| description:`Export 'b' from module './a'`, | ||
| newFileContent:{ | ||
| "/a.ts":`class a{}, export class b{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is producing invalid syntax. I think we need to fallback toexport { b }; in this case.
Uh oh!
There was an error while loading.Please reload this page.
Fixesmicrosoft#37440
If:
The import is from a .d.ts file, do nothing.
The declaration is a single variable (e.g.
let a = 1): update toexport let a = 1The declaration is a single function (e.g.
function a(){...}): update toexport function a(){...}The declaration is in a list and contains multiple variables (e.g.
let a = 1, b = 2)export {...}found in the file: create anexport {...}and add to thatexport {...}found in the file: add to the lastexport {...}Testing performed
Local testing + tests added
Additional context
Based on thisclosed PR
I think I addressed all the comments exceptthis, which seems reasonable to me!