Skip to content

Optional configuration for multiple imports per line and strictly one import per line. #50

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.

Sign up for GitHub

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 6 commits into from
Feb 26, 2018
Merged

Optional configuration for multiple imports per line and strictly one import per line. #50

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
24d465b
Optional configuration for multiple imports per line
Feb 6, 2018
c1e84da
Fix Lint and unit tests.
Feb 6, 2018
8a1d5e5
Merge branch 'develop' into develop
buehler Feb 6, 2018
aed9a25
Restructuring import organization.
Feb 10, 2018
59e1013
Set sourceMaps to false again as per review.
Feb 10, 2018
6dda58a
Updated tests.
Feb 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/tsconfig.base.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"outDir": "../build",
"rootDir": "../src",
"declaration": true,
"sourceMap": false,
"sourceMap": true,
Copy link
Owner

Choose a reason for hiding this comment

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

Please not in the base config. The tsconfig.jsonconfiguration does specify sourceMap: true. The base should not change that since the production config does not overwrite the setting again.

-> false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

"importHelpers": true,
"strictNullChecks": true,
"experimentalDecorators": true,
Expand Down
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
},
"homepage": "https://github.com/TypeScript-Heroes/node-typescript-parser#readme",
"devDependencies": {
"@types/lodash-es": "^4.17.0",
"@types/jest": "^21.1.8",
"@types/mock-fs": "^3.6.30",
"@types/node": "^8.0.57",
Expand All @@ -45,7 +46,8 @@
"typedoc": "^0.9.0"
},
"dependencies": {
"lodash": "^4.17.4",
"lodash": "^4.17.5",
Copy link
Owner

Choose a reason for hiding this comment

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

so if you want to use lodash-es instead of lodash (which is fine by me, since I didn't knew it existed) you can remove the other one right? or are both needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both are needed, I only had to add these because I was facing errors in running npm run develop. Right now lodash imports are untouched. At some point I would change those imports to use lodash-es. With that I tihnk we can eventually get treeshaking benefit.

Copy link
Owner

Choose a reason for hiding this comment

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

Alright

"lodash-es": "^4.17.5",
"tslib": "^1.8.1",
"typescript": "^2.6.2"
}
Expand Down
8 changes: 8 additions & 0 deletions src/code-generators/TypescriptGenerationOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ export interface TypescriptGenerationOptions {
*/
spaceBraces: boolean;

/**
* The wrapping method to be used in multiline imports.
*
* @type {'MULTIPLE_IMPORTS_PER_LINE' | 'ONE_IMPORT_PER_LINE'}
* @memberof TypescriptGenerationOptions
*/
multiLineWrapMethod: 'ONE_IMPORT_PER_LINE' | 'MULTIPLE_IMPORTS_PER_LINE';
Copy link
Owner

Choose a reason for hiding this comment

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

I'd use a string enum for this particular case:
This way, it should be exported and is easy extendable (and more convenient to use in an IDE)

export enum MultiLineImportRule {
  onePerLine = 'onePerLine',
  multiplePerLine = 'multiplePerLine',
}

With this method, we can implement this feature as a normal config in typescript hero and use the enum directly (as string value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.


/**
* The threshold where an import is written as multiline.
*
Expand Down
64 changes: 48 additions & 16 deletions src/code-generators/typescript-generators/namedImport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ const multiLineImport = stringTemplate`import ${3}{
${0}${1}
} from ${2}`;

const aliasOnlyMultiLineImport = stringTemplate`import ${0}
from ${1}`;

/**
* Sort function for symbol specifiers. Does sort after the specifiers name (to lowercase).
*
Expand Down Expand Up @@ -45,6 +48,7 @@ export function generateNamedImport(
stringQuoteStyle,
spaceBraces,
tabSize,
multiLineWrapMethod,
multiLineWrapThreshold,
multiLineTrailingComma,
}: TypescriptGenerationOptions,
Expand All @@ -53,26 +57,54 @@ export function generateNamedImport(
const lib = `${stringQuoteStyle}${imp.libraryName}${stringQuoteStyle}${eol}`;

const specifiers = imp.specifiers.sort(specifierSort).map(o => generateSymbolSpecifier(o)).join(', ');
let importSpecifiers = `${space}${specifiers}${space}`;
if (importSpecifiers.trim().length === 0) {
importSpecifiers = ' ';
}

const importString = importTemplate(
getImportSpecifiers(imp, spaceBraces),
lib,
);
let retVal:string = '';
Copy link
Owner

Choose a reason for hiding this comment

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

missing space in code formatting

Copy link
Owner

Choose a reason for hiding this comment

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

And please use the whole name returnValue or generatedImport or something like that. I'm not a big fan of abbreviations since we're not coding C or something ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup.


if (importString.length > multiLineWrapThreshold) {
if (specifiers.length > multiLineWrapThreshold) {
Copy link
Owner

Choose a reason for hiding this comment

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

This actually changes the behaviour of the multi line wrap threshold. before, the generated string was checked for his length. Now only the specifiers.

Or did I misread something?

Copy link
Owner

Choose a reason for hiding this comment

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

It should behave like before, or we need to introduce a BREAKING CHANGE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually intended this change, because I thought that it is the right thing to do, didn't realize that it could be considered a Breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I will rever that part for now. Its small thing to change later if think that it should be done.

const spacings = Array(tabSize + 1).join(' ');
return multiLineImport(
imp.specifiers.sort(specifierSort).map(o => `${spacings}${generateSymbolSpecifier(o)}`).join(',\n'),
multiLineTrailingComma ? ',' : '',
`${stringQuoteStyle}${imp.libraryName}${stringQuoteStyle}${eol}`,
imp.defaultAlias ? `${imp.defaultAlias}, ` : '',
const sortedImportSpecifiers: SymbolSpecifier[] = imp.specifiers.sort(specifierSort);
let importSpecifierStrings: string = '';

if (multiLineWrapMethod === 'MULTIPLE_IMPORTS_PER_LINE') {
Copy link
Owner

Choose a reason for hiding this comment

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

Please use the string enum, as mentioned above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes.

importSpecifierStrings = sortedImportSpecifiers.reduce(
(acc, curr) => {
const symbolSpecifier: string = generateSymbolSpecifier(curr);
const dist: number = acc.out.length - acc.lastWrapOffset + symbolSpecifier.length;
const needsWrap: boolean = dist >= multiLineWrapThreshold;
return {
out: acc.out + (needsWrap ? `,\n${spacings}` : (acc.out.length ? `, ` : `${spacings}`)) +
symbolSpecifier,
lastWrapOffset: acc.lastWrapOffset + (needsWrap ? dist : 0),
};
},
{
out: '',
lastWrapOffset: 0,
},
).out;
} else {
// For 'ONE_IMPORT_PER_LINE' which also happens to be the default case.
importSpecifierStrings = sortedImportSpecifiers.map(o => `${spacings}${generateSymbolSpecifier(o)}`).join(',\n');
}
if (imp.specifiers.length > 0) {
retVal = multiLineImport(
importSpecifierStrings,
multiLineTrailingComma ? ',' : '',
`${stringQuoteStyle}${imp.libraryName}${stringQuoteStyle}${eol}`,
imp.defaultAlias ? `${imp.defaultAlias}, ` : '',
);
} else {
retVal = aliasOnlyMultiLineImport(
imp.defaultAlias ? `${imp.defaultAlias}, ` : '',
`${stringQuoteStyle}${imp.libraryName}${stringQuoteStyle}${eol}`,
);
}
} else {
retVal = importTemplate(
getImportSpecifiers(imp, spaceBraces),
lib,
);
}
return importString;
return retVal;
}

function getImportSpecifiers(namedImport: NamedImport, spaceBraces: boolean): string {
Expand Down
26 changes: 26 additions & 0 deletions test/code-generators/TypescriptCodeGenerator.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ multiLineNamedImport.specifiers = [
new SymbolSpecifier('spec13'),
new SymbolSpecifier('spec14'),
new SymbolSpecifier('spec15'),
new SymbolSpecifier('spec16'),
new SymbolSpecifier('spec17'),
new SymbolSpecifier('spec18'),
];

const defaultImport = new NamedImport('defaultImport');
Expand All @@ -54,6 +57,7 @@ describe('TypescriptCodeGenerator', () => {
const defaultOptions: TypescriptGenerationOptions = {
eol: ';',
multiLineTrailingComma: true,
multiLineWrapMethod: 'ONE_IMPORT_PER_LINE',
multiLineWrapThreshold: 125,
spaceBraces: true,
stringQuoteStyle: `'`,
Expand All @@ -62,6 +66,16 @@ describe('TypescriptCodeGenerator', () => {
const impOptions: TypescriptGenerationOptions = {
eol: ';',
multiLineTrailingComma: true,
multiLineWrapMethod: 'ONE_IMPORT_PER_LINE',
multiLineWrapThreshold: 125,
spaceBraces: true,
stringQuoteStyle: `"`,
tabSize: 2,
};
const impOptions_multipleImportsPerLine: TypescriptGenerationOptions = {
eol: ';',
multiLineTrailingComma: true,
multiLineWrapMethod: 'MULTIPLE_IMPORTS_PER_LINE',
multiLineWrapThreshold: 125,
spaceBraces: true,
stringQuoteStyle: `"`,
Expand Down Expand Up @@ -139,6 +153,18 @@ describe('TypescriptCodeGenerator', () => {
expect(generator.generate(imp)).toMatchSnapshot();
});

it(`should generate multiple imports per line for ${imp.constructor.name} with single quote`, () => {
const generator = new TypescriptCodeGenerator(impOptions_multipleImportsPerLine);

expect(generator.generate(imp)).toMatchSnapshot();
});

it(`should generate multiple imports per line for ${imp.constructor.name} with double quote`, () => {
const generator = new TypescriptCodeGenerator(impOptions_multipleImportsPerLine);

expect(generator.generate(imp)).toMatchSnapshot();
});

}

it('should throw on non generatable element', () => {
Expand Down
74 changes: 74 additions & 0 deletions test/code-generators/__snapshots__/TypescriptCodeGenerator.spec.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,61 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`TypescriptCodeGenerator should generate multiple imports per line for ExternalModuleImport with double quote 1`] = `"import externalAlias = require(\\"externalModuleLib\\");"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for ExternalModuleImport with single quote 1`] = `"import externalAlias = require(\\"externalModuleLib\\");"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with double quote 1`] = `"import { spec1, spec2 as alias2 } from \\"namedLib\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with double quote 2`] = `
"import {
spec1, spec10, spec11, spec12, spec13, spec14, spec15, spec16, spec17, spec18, spec2, spec3, spec4, spec5, spec6, spec7,
spec8, spec9,
} from \\"multiLineNamedLib\\";"
`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with double quote 3`] = `"import { } from \\"emptyImport\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with double quote 4`] = `"import Default from \\"defaultImport\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with double quote 5`] = `"import Default, { spec1, spec2 as alias2 } from \\"defaultWithNamedImport\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with double quote 6`] = `
"import Default, {
spec1, spec10, spec11, spec12, spec13, spec14, spec15, spec16, spec17, spec18, spec2, spec3, spec4, spec5, spec6, spec7,
spec8, spec9,
} from \\"defaultWithNamedMultilineImport\\";"
`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with single quote 1`] = `"import { spec1, spec2 as alias2 } from \\"namedLib\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with single quote 2`] = `
"import {
spec1, spec10, spec11, spec12, spec13, spec14, spec15, spec16, spec17, spec18, spec2, spec3, spec4, spec5, spec6, spec7,
spec8, spec9,
} from \\"multiLineNamedLib\\";"
`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with single quote 3`] = `"import { } from \\"emptyImport\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with single quote 4`] = `"import Default from \\"defaultImport\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with single quote 5`] = `"import Default, { spec1, spec2 as alias2 } from \\"defaultWithNamedImport\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamedImport with single quote 6`] = `
"import Default, {
spec1, spec10, spec11, spec12, spec13, spec14, spec15, spec16, spec17, spec18, spec2, spec3, spec4, spec5, spec6, spec7,
spec8, spec9,
} from \\"defaultWithNamedMultilineImport\\";"
`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamespaceImport with double quote 1`] = `"import * as namespaceAlias from \\"namespaceLib\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for NamespaceImport with single quote 1`] = `"import * as namespaceAlias from \\"namespaceLib\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for StringImport with double quote 1`] = `"import \\"stringLib\\";"`;

exports[`TypescriptCodeGenerator should generate multiple imports per line for StringImport with single quote 1`] = `"import \\"stringLib\\";"`;

exports[`TypescriptCodeGenerator should generate the correct code for ExternalModuleImport 1`] = `"import externalAlias = require('externalModuleLib');"`;

exports[`TypescriptCodeGenerator should generate the correct code for ExternalModuleImport with double quote 1`] = `"import externalAlias = require(\\"externalModuleLib\\");"`;
Expand Down Expand Up @@ -107,6 +163,9 @@ exports[`TypescriptCodeGenerator should generate the correct code for NamedImpor
spec13,
spec14,
spec15,
spec16,
spec17,
spec18,
spec2,
spec3,
spec4,
Expand All @@ -133,6 +192,9 @@ exports[`TypescriptCodeGenerator should generate the correct code for NamedImpor
spec13,
spec14,
spec15,
spec16,
spec17,
spec18,
spec2,
spec3,
spec4,
Expand All @@ -155,6 +217,9 @@ exports[`TypescriptCodeGenerator should generate the correct code for NamedImpor
spec13,
spec14,
spec15,
spec16,
spec17,
spec18,
spec2,
spec3,
spec4,
Expand All @@ -181,6 +246,9 @@ exports[`TypescriptCodeGenerator should generate the correct code for NamedImpor
spec13,
spec14,
spec15,
spec16,
spec17,
spec18,
spec2,
spec3,
spec4,
Expand All @@ -203,6 +271,9 @@ exports[`TypescriptCodeGenerator should generate the correct code for NamedImpor
spec13,
spec14,
spec15,
spec16,
spec17,
spec18,
spec2,
spec3,
spec4,
Expand All @@ -229,6 +300,9 @@ exports[`TypescriptCodeGenerator should generate the correct code for NamedImpor
spec13,
spec14,
spec15,
spec16,
spec17,
spec18,
spec2,
spec3,
spec4,
Expand Down