Feat#Provide a quick-fix for non-exported imports

[Richard-Lynch]

Fixes microsoft#37440

If:

• The import is from a .d.ts file, do nothing.

• The declaration is a single variable (e.g. let a = 1): update to export let a = 1

• The declaration is a single function (e.g. function a(){...}): update to export function a(){...}

• The declaration is in a list and contains multiple variables (e.g. let a = 1, b = 2)

  • No export {...} found in the file: create an export {...} and add to that
  • Multiple export {...} found in the file: add to the last export {...}

Testing performed
Local testing + tests added

Additional context
Based on this closed PR

I think I addressed all the comments except this, which seems reasonable to me!

[Richard-Lynch]

Maybe worth adding (at)DanielRosenwasser and (at)sandersn when this is added to the TS repo

[mkubilayk]

Looking very good - some minor comments.

[mkubilayk]

Do we need special handling for class declarations as well?

[Richard-Lynch]

class is captured/handled with isDeclarationStatement but I've added some tests and simplified that logic

[mkubilayk]

Should we check if this is a type-only export? My understanding is that this would change:

export type { existingThing };

to

export { existingThing, newThing };

which may not be desired.

[Richard-Lynch]

nice! yes, this is a good test case, adding now

[mkubilayk]

We should skip re-export statements here: export { a } from "./something";.

[Richard-Lynch]

yup nice, added

[mkubilayk]

Suggested change

	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.

[Richard-Lynch]

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!

[mkubilayk]

Suggested change

	/*exportClause*/ factory.createNamedExports([
	factory.createExportSpecifier(
	/*isTypeOnly*/ false,
	/*propertyName*/ undefined,
	node
	),
	]),
	factory.createNamedExports([
	factory.createExportSpecifier(
	/*isTypeOnly*/ false,
	/*propertyName*/ undefined,
	node
	),
	]),

[mkubilayk]

Suggested change

	/*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"

[mkubilayk]

Would be good to add some test cases where source file has some exports that we cannot use:

export type { a };
export { b } from "./b";
export * as c from "./c";
// ...

[Richard-Lynch]

done

[acutmore]

Suggested change

	"Export '{0}' from module '{1}'": {
	"Export \"{0}\" from module \"{1}\"": {

minor, seems to match the other ones above this.

[Richard-Lynch]

yup nice, done

[Richard-Lynch]

hmm, the Remove import from './b' uses single quotes, what do you think?

[Richard-Lynch]

it looks like there are only 4 instances using double quotes, that seems like a mistake

[dragomirtitian]

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"

[Richard-Lynch]

Agreed, I changed to Export all missing members from modules

[dragomirtitian]

I would like to see some tests on overloaded functions as well.

[Richard-Lynch]

Good shout! do you think the export should be on the implemented function or in a named export? 🤔

[Richard-Lynch]

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?

[mkubilayk]

I think a separate export statement (e.g. export { add }) would be acceptable for function overloads.

[mkubilayk]

This is producing invalid syntax. I think we need to fallback to export { b }; in this case.