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

Remove unused imported types for generated typescript. #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SethCopeland
Copy link

We are importing all the types from generated types file when we only need to import a subset of them. While all of them might be used, they are used implicitly in some cases, so this PR is meant to ensure we only import the types that we use explicitly.

For the client this means only including IServiceName, the IXyzRequests and the IXyzResponses types, and excluding the dtos defined with the data keyword in the fsd file.

For the server this means only including the IServiceName and the IXyzRequest.

@@ -61,7 +63,7 @@ protected override CodeGenOutput GenerateOutputCore(ServiceInfo service)

code.WriteLine();
WriteJsDoc(code, service);
typeNames.Add($"I{capModuleName}");
requestTypeNames.Add($"I{capModuleName}");
Copy link
Author

Choose a reason for hiding this comment

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

This isn't technically a request type, but wherever I needed the request types I also needed this so I included it here.

var typeNames = new List<string>();
var requestTypeNames = new List<string>();
var responseTypeNames = new List<string>();
var dtoTypeNames = new List<string>();
Copy link
Author

Choose a reason for hiding this comment

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

I broke up the types and made this before I realized this wasn't actually being used anywhere.

@@ -119,7 +121,7 @@ protected override CodeGenOutput GenerateOutputCore(ServiceInfo service)

if (TypeScript)
{
WriteImports(code, typeNames, $"./{Uncapitalize(moduleName)}Types");
WriteImports(code, requestTypeNames.Concat(responseTypeNames).ToList(), $"./{Uncapitalize(moduleName)}Types");
Copy link
Author

Choose a reason for hiding this comment

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

I'm assuming the order in which we import things is arbitrary, and we don't care that all the requests are imported first and then response, as opposed to what it was previously import { IRequestOne, IResponseOne, ...IRequestN, IResponseN }

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should sort them. Maybe WriteImports should sort them.

Copy link
Author

Choose a reason for hiding this comment

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

I sorted them alphabetically which maybe is better. Though maybe even better would be to sort them by where they would be used in the file. Which would probably more closely correspond to where they are specified in the fsd file is my guess.

Copy link
Author

Choose a reason for hiding this comment

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

I could do something like this and replace all this code

		private static void WriteImports(CodeWriter code, IReadOnlyList<string> imports, string from, string fileMinusImports)
		{
			var neededImports = imports.Where(x => fileMinusImports.Contains(x)).ToList();
			if (neededImports.Count != 0)
				code.WriteLine($"import {{ {string.Join(", ", neededImports.OrderBy(x => fileMinusImports.IndexOf(x)))} }} from '{from}';");
		}

though I'd actually have to use a regular expression instead of Contains and IndexOf to ensure IFoo doesn't match IFooBar and I'd have to create a separate StringWriter to get fileMinusImports, which all seems worse.

@ddunkin
Copy link
Contributor

ddunkin commented Mar 11, 2019

Is this to satisfy some lint rule? I don’t object to the change. I’m not able to really test it until next week. Please run the CodeGen target to update the example service test files.

@SethCopeland
Copy link
Author

Many of the tsconfig's specify no unused locals. So it is fixing in some sense a possible compiler error.
I've gotten errors in my consuming tools because the client includes a type definition file with unused imports. We can just turn off that tsconfig setting and use eslint to handle the unused locals case though.

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