From 3271c7b18f37d3aaf7a9b5bda8c2bb1d56af2c70 Mon Sep 17 00:00:00 2001 From: Chris Krycho Date: Thu, 3 Mar 2022 07:59:49 -0700 Subject: [PATCH 1/6] Update tsconfig and corresponding docs Our generated blueprint was *years* out of date, and between the changes we made in the types over the last year or so and the guidance we have given in the TS RFCs we are now both *able* and *required* to set `strict: true` as well as to opt into `noUncheckedIndexedAccess` and to disable `allowSyntheticDefaultImports` and `esModuleInterop`. Additionally, this provided a nice opportunity to document the choices we have made so folks have something to latch onto when looking at the config we generate for them, and to update the config to target the current latest versions of the `target` and `module` fields. --- .../ember-cli-typescript/tsconfig.json | 62 ++++++++++++++++--- docs/ts/using-ts-effectively.md | 30 +++++---- 2 files changed, 69 insertions(+), 23 deletions(-) diff --git a/blueprint-files/ember-cli-typescript/tsconfig.json b/blueprint-files/ember-cli-typescript/tsconfig.json index d1959549f..93b046ce6 100644 --- a/blueprint-files/ember-cli-typescript/tsconfig.json +++ b/blueprint-files/ember-cli-typescript/tsconfig.json @@ -1,25 +1,67 @@ { "compilerOptions": { - "target": "es2020", - "allowJs": true, + "target": "ES2022", + "module": "ES2022", "moduleResolution": "node", - "allowSyntheticDefaultImports": true, - "noImplicitAny": true, - "noImplicitThis": true, - "alwaysStrict": true, - "strictNullChecks": true, - "strictPropertyInitialization": true, + + // Trying to check Ember apps and addons with `allowJs: true` is a recipe + // for many unresolveable type errors, because with *considerable* extra + // configuration it ends up including many files which are *not* valid and + // cannot be: they *appear* to be resolve-able to TS, but are in fact not in + // valid Node-resolveable locations and may not have TS-ready types. This + // will likely improve over time + "allowJs": false, + + // --- TS for SemVer Types compatibility + // Strictness settings -- you should *not* change these: Ember code is not + // guaranteed to type check with these set to looser values. + "strict": true, + "noUncheckedIndexedAccess": true, + + // Interop: these are viral and will require anyone downstream of your + // package to *also* set them to true. If you *must* enable them to consume + // an upstream package, you should + "allowSyntheticDefaultImports": false, + "esModuleInterop": false, + + // --- Lint-style rules + + // You should feel free to change these, especially if you are already + // covering them via linting (e.g. with @typescript-eslint). "noFallthroughCasesInSwitch": true, "noUnusedLocals": true, "noUnusedParameters": true, "noImplicitReturns": true, + "noPropertyAccessFromIndexSignature": true, + + // --- Compilation/integration settings + // Setting `noEmitOnError` here allows ember-cli-typescript to catch errors + // and inject them into Ember CLI's build error reporting, which provides + // nice feedback for when "noEmitOnError": true, + + // We use Babel for emitting runtime code, because it's very important that + // we always and only use the same transpiler for non-stable features, in + // particular decorators. If you were to change this to `true`, it could + // lead to accidentally generating code with `tsc` instead of Babel, and + // could thereby result in broken code at runtime. "noEmit": true, + + // Ember makes heavy use of decorators; TS does not support them at all + // without this flag. + "experimentalDecorators": true, + + // Support generation of source maps. Note: you must *also* enable source + // maps in your `ember-cli-babel` config and/or `babel.config.js`. + "declaration": true, + "declarationMap": true, "inlineSourceMap": true, "inlineSources": true, + + // The combination of `baseUrl` with `paths` allows Ember's classic package + // layout, which is not resolveable with the Node resolution algorithm, to + // work with TypeScript. "baseUrl": ".", - "module": "es6", - "experimentalDecorators": true, "paths": <%= pathsFor(dasherizedPackageName) %> }, "include": <%= includes %> diff --git a/docs/ts/using-ts-effectively.md b/docs/ts/using-ts-effectively.md index 54257d954..344d5b56a 100644 --- a/docs/ts/using-ts-effectively.md +++ b/docs/ts/using-ts-effectively.md @@ -8,24 +8,28 @@ Some specific tips for success on the technical front: First, use the _strictest_ strictness settings that our typings allow (currently all strictness settings except `strictFunctionTypes`). While it may be tempting to start with the _loosest_ strictness settings and then to tighten them down as you go, this will actually mean that "getting your app type-checking" will become a repeated process—getting it type-checking with every new strictness setting you enable—rather than something you do just once. -The full recommended _strictness_ settings in your `"compilerOptions"` hash: +The full recommended _strictness_ settings in your `"compilerOptions"` hash (which are also the settings generated by the ember-cli-typescript blueprint): -```json +```json5 { - "noImplicitAny": true, - "noImplicitThis": true, - "alwaysStrict": true, - "strictNullChecks": true, - "strictPropertyInitialization": true, - "noFallthroughCasesInSwitch": true, - "noUnusedLocals": true, - "noUnusedParameters": true, - "noImplicitReturns": true, - "noUncheckedIndexedAccess": true, + "compilerOptions": { + // Strictness settings -- you should *not* change these: Ember code is not + // guaranteed to type check with these set to looser values. + "strict": true, + "noUncheckedIndexedAccess": true, + + // You should feel free to change these, especially if you are already + // covering them via linting (e.g. with @typescript-eslint). + "noFallthroughCasesInSwitch": true, + "noUnusedLocals": true, + "noUnusedParameters": true, + "noImplicitReturns": true, + "noPropertyAccessFromIndexSignature": true, + } } ``` -A good approach is to start at your "leaf" files (the ones that don't import anything else from your app, only Ember types) and then work your way back inward toward the most core types that are used everywhere. Often the highest-value modules are your Ember Data models and any core services that are used everywhere else in the app – and those are also the ones that tend to have the most cascading effects (having to update _tons_ of other places in your app) when you type them later in the process. +A good approach is to start at your "leaf" modules (the ones that don't import anything else from your app, only Ember or third-party types) and then work your way back inward toward the most core moduels that are used everywhere. Often the highest-value modules are your Ember Data models and any core services that are used everywhere else in the app – and those are also the ones that tend to have the most cascading effects (having to update _tons_ of other places in your app) when you type them later in the process. Finally, leave `"noEmitOnError": true` (the default) in the `"compilerOptions"` hash in your `tsconfig.json`. This will fail your build if you have type errors, which gives you the fastest feedback as you add types. From dafae6dabf98ba5417469a732acca1bdfdb11574 Mon Sep 17 00:00:00 2001 From: Chris Krycho Date: Thu, 3 Mar 2022 09:17:02 -0700 Subject: [PATCH 2/6] Docs: fix a type on docs/ts/using-ts-effectively.md Co-authored-by: Dan Freeman --- docs/ts/using-ts-effectively.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/ts/using-ts-effectively.md b/docs/ts/using-ts-effectively.md index 344d5b56a..37a7711eb 100644 --- a/docs/ts/using-ts-effectively.md +++ b/docs/ts/using-ts-effectively.md @@ -29,7 +29,7 @@ The full recommended _strictness_ settings in your `"compilerOptions"` hash (whi } ``` -A good approach is to start at your "leaf" modules (the ones that don't import anything else from your app, only Ember or third-party types) and then work your way back inward toward the most core moduels that are used everywhere. Often the highest-value modules are your Ember Data models and any core services that are used everywhere else in the app – and those are also the ones that tend to have the most cascading effects (having to update _tons_ of other places in your app) when you type them later in the process. +A good approach is to start at your "leaf" modules (the ones that don't import anything else from your app, only Ember or third-party types) and then work your way back inward toward the most core modules that are used everywhere. Often the highest-value modules are your Ember Data models and any core services that are used everywhere else in the app – and those are also the ones that tend to have the most cascading effects (having to update _tons_ of other places in your app) when you type them later in the process. Finally, leave `"noEmitOnError": true` (the default) in the `"compilerOptions"` hash in your `tsconfig.json`. This will fail your build if you have type errors, which gives you the fastest feedback as you add types. From 7d490d30fdb3ae7a5d685e7cb4e465cfe56f582f Mon Sep 17 00:00:00 2001 From: Chris Krycho Date: Thu, 3 Mar 2022 09:20:00 -0700 Subject: [PATCH 3/6] Clarify/elaborate on comments in tsconfig.json --- .../ember-cli-typescript/tsconfig.json | 21 +++++++++++-------- ts/tests/commands/precompile-test.ts | 7 ++++--- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/blueprint-files/ember-cli-typescript/tsconfig.json b/blueprint-files/ember-cli-typescript/tsconfig.json index 93b046ce6..e6bedc151 100644 --- a/blueprint-files/ember-cli-typescript/tsconfig.json +++ b/blueprint-files/ember-cli-typescript/tsconfig.json @@ -1,7 +1,7 @@ { "compilerOptions": { - "target": "ES2022", - "module": "ES2022", + "target": "ES2021", + "module": "ES2020", "moduleResolution": "node", // Trying to check Ember apps and addons with `allowJs: true` is a recipe @@ -20,18 +20,21 @@ // Interop: these are viral and will require anyone downstream of your // package to *also* set them to true. If you *must* enable them to consume - // an upstream package, you should + // an upstream package, you should document that for downstream consumers to + // be aware of. + // + // These *are* safe for apps to enable, since they do not *have* downstream + // consumers; but leaving them off is still preferred when possible, since + // it makes it easier to switch between apps and addons and have the same + // rules for what can be imported and how. "allowSyntheticDefaultImports": false, "esModuleInterop": false, // --- Lint-style rules - // You should feel free to change these, especially if you are already - // covering them via linting (e.g. with @typescript-eslint). - "noFallthroughCasesInSwitch": true, - "noUnusedLocals": true, - "noUnusedParameters": true, - "noImplicitReturns": true, + // TypeScript also supplies some lint-style checks; nearly all of them are + // better handled by ESLint with the `@typescript-eslint`. This one is more + // like a safety check, though, so we leave it on. "noPropertyAccessFromIndexSignature": true, // --- Compilation/integration settings diff --git a/ts/tests/commands/precompile-test.ts b/ts/tests/commands/precompile-test.ts index 42247a470..d3351ed14 100644 --- a/ts/tests/commands/precompile-test.ts +++ b/ts/tests/commands/precompile-test.ts @@ -3,6 +3,7 @@ import * as path from 'path'; import { hook } from 'capture-console'; import ember from 'ember-cli-blueprint-test-helpers/lib/helpers/ember'; import blueprintHelpers from 'ember-cli-blueprint-test-helpers/helpers'; +import ts from 'typescript'; const setupTestHooks = blueprintHelpers.setupTestHooks; const emberNew = blueprintHelpers.emberNew; @@ -63,8 +64,8 @@ describe('Acceptance: ts:precompile command', function () { fs.writeFileSync('src/test-file.ts', `export const testString: string = 'hello';`); let pkg = fs.readJsonSync('package.json'); - let tsconfig = fs.readJSONSync('tsconfig.json'); - tsconfig.include.push('src'); + let tsconfig = ts.readConfigFile('tsconfig.json', ts.sys.readFile).config; + tsconfig.tsconfig.include.push('src'); tsconfig.compilerOptions.paths[`${pkg.name}/src/*`] = ['src/*']; fs.writeJSONSync('tsconfig.json', tsconfig); @@ -85,7 +86,7 @@ describe('Acceptance: ts:precompile command', function () { ); let pkg = fs.readJsonSync('package.json'); - let tsconfig = fs.readJSONSync('tsconfig.json'); + let tsconfig = ts.readConfigFile('tsconfig.json', ts.sys.readFile).config; tsconfig.include.push('src'); tsconfig.compilerOptions.paths[`${pkg.name}/*`] = ['addon-test-support/*']; fs.writeJSONSync('tsconfig.json', tsconfig); From c265d7424178408c712c851598f1512aada01fdd Mon Sep 17 00:00:00 2001 From: Chris Krycho Date: Thu, 3 Mar 2022 09:42:16 -0700 Subject: [PATCH 4/6] Handle comments in tsconfig in precompile tests --- ts/tests/commands/precompile-test.ts | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/ts/tests/commands/precompile-test.ts b/ts/tests/commands/precompile-test.ts index d3351ed14..0094528ae 100644 --- a/ts/tests/commands/precompile-test.ts +++ b/ts/tests/commands/precompile-test.ts @@ -27,7 +27,9 @@ describe('Acceptance: ts:precompile command', function () { let declaration = file('test-file.d.ts'); expect(declaration).to.exist; - expect(declaration.content.trim()).to.equal(`export declare const testString: string;`); + expect(declaration.content.trim()).to.equal( + `export declare const testString: string;\n//# sourceMappingURL=test-file.d.ts.map` + ); }); it('generates nothing from the app tree', async () => { @@ -65,7 +67,7 @@ describe('Acceptance: ts:precompile command', function () { let pkg = fs.readJsonSync('package.json'); let tsconfig = ts.readConfigFile('tsconfig.json', ts.sys.readFile).config; - tsconfig.tsconfig.include.push('src'); + tsconfig.include.push('src'); tsconfig.compilerOptions.paths[`${pkg.name}/src/*`] = ['src/*']; fs.writeJSONSync('tsconfig.json', tsconfig); @@ -73,7 +75,9 @@ describe('Acceptance: ts:precompile command', function () { let declaration = file('src/test-file.d.ts'); expect(declaration).to.exist; - expect(declaration.content.trim()).to.equal(`export declare const testString: string;`); + expect(declaration.content.trim()).to.equal( + `export declare const testString: string;\n//# sourceMappingURL=test-file.d.ts.map` + ); }); }); @@ -95,7 +99,9 @@ describe('Acceptance: ts:precompile command', function () { let declaration = file('test-file.d.ts'); expect(declaration).to.exist; - expect(declaration.content.trim()).to.equal(`export declare const testString: string;`); + expect(declaration.content.trim()).to.equal( + `export declare const testString: string;\n//# sourceMappingURL=test-file.d.ts.map` + ); }); }); @@ -121,7 +127,9 @@ describe('Acceptance: ts:precompile command', function () { return ember(['ts:precompile']).then(() => { const declaration = file('test-support/test-file.d.ts'); expect(declaration).to.exist; - expect(declaration.content.trim()).to.equal(`export declare const testString: string;`); + expect(declaration.content.trim()).to.equal( + `export declare const testString: string;\n//# sourceMappingURL=test-file.d.ts.map` + ); }); }); @@ -155,12 +163,14 @@ describe('Acceptance: ts:precompile command', function () { return ember(['ts:precompile']).then(() => { const componentDecl = file('components/my-component.d.ts'); expect(componentDecl).to.exist; - expect(componentDecl.content.trim()).to.equal(`export declare const testString: string;`); + expect(componentDecl.content.trim()).to.equal( + `export declare const testString: string;\n//# sourceMappingURL=my-component.d.ts.map` + ); const testSupportDecl = file('test-support/test-file.d.ts'); expect(testSupportDecl).to.exist; expect(testSupportDecl.content.trim()).to.equal( - `export declare const anotherTestString: string;` + `export declare const anotherTestString: string;\n//# sourceMappingURL=test-file.d.ts.map` ); }); }); From 4b914f8ab58f399ea2831f0505b6af66ed2da017 Mon Sep 17 00:00:00 2001 From: Chris Krycho Date: Thu, 3 Mar 2022 10:16:24 -0700 Subject: [PATCH 5/6] Allow for case where we don't need to move package --- .../blueprints/ember-cli-typescript-test.ts | 11 ++++---- ts/tests/helpers/stash-published-version.ts | 26 +++++++++++++++++-- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/ts/tests/blueprints/ember-cli-typescript-test.ts b/ts/tests/blueprints/ember-cli-typescript-test.ts index 81dbcf327..4727e4d76 100644 --- a/ts/tests/blueprints/ember-cli-typescript-test.ts +++ b/ts/tests/blueprints/ember-cli-typescript-test.ts @@ -5,6 +5,7 @@ import path from 'path'; import helpers from 'ember-cli-blueprint-test-helpers/helpers'; import chaiHelpers from 'ember-cli-blueprint-test-helpers/chai'; import Blueprint from 'ember-cli/lib/models/blueprint'; +import ts from 'typescript'; import { setupPublishedVersionStashing } from '../helpers/stash-published-version'; import ects from '../../blueprints/ember-cli-typescript/index'; @@ -71,7 +72,7 @@ describe('Acceptance: ember-cli-typescript generator', function () { const tsconfig = file('tsconfig.json'); expect(tsconfig).to.exist; - const tsconfigJson = JSON.parse(tsconfig.content); + const tsconfigJson = ts.parseConfigFileTextToJson('tsconfig.json', tsconfig.content).config; expect(tsconfigJson.compilerOptions.paths).to.deep.equal({ 'my-app/tests/*': ['tests/*'], 'my-app/*': ['app/*'], @@ -121,7 +122,7 @@ describe('Acceptance: ember-cli-typescript generator', function () { const tsconfig = file('tsconfig.json'); expect(tsconfig).to.exist; - const tsconfigJson = JSON.parse(tsconfig.content); + const tsconfigJson = ts.parseConfigFileTextToJson('tsconfig.json', tsconfig.content).config; expect(tsconfigJson.compilerOptions.paths).to.deep.equal({ 'dummy/tests/*': ['tests/*'], 'dummy/*': ['tests/dummy/app/*', 'app/*'], @@ -197,7 +198,7 @@ describe('Acceptance: ember-cli-typescript generator', function () { const tsconfig = file('tsconfig.json'); expect(tsconfig).to.exist; - const json = JSON.parse(tsconfig.content); + const json = ts.parseConfigFileTextToJson('tsconfig.json', tsconfig.content).config; expect(json.compilerOptions.paths).to.deep.equal({ 'my-app/tests/*': ['tests/*'], 'my-app/*': ['app/*', 'lib/my-addon-1/app/*', 'lib/my-addon-2/app/*'], @@ -243,7 +244,7 @@ describe('Acceptance: ember-cli-typescript generator', function () { const tsconfig = file('tsconfig.json'); expect(tsconfig).to.exist; - const json = JSON.parse(tsconfig.content); + const json = ts.parseConfigFileTextToJson('tsconfig.json', tsconfig.content).config; expect(json.compilerOptions.paths).to.deep.equal({ 'my-app/tests/*': ['tests/*'], 'my-app/mirage/*': ['mirage/*'], @@ -269,7 +270,7 @@ describe('Acceptance: ember-cli-typescript generator', function () { const tsconfig = file('tsconfig.json'); expect(tsconfig).to.exist; - const json = JSON.parse(tsconfig.content); + const json = ts.parseConfigFileTextToJson('tsconfig.json', tsconfig.content).config; expect(json.compilerOptions.paths).to.deep.equal({ 'dummy/tests/*': ['tests/*'], 'dummy/mirage/*': ['tests/dummy/mirage/*'], diff --git a/ts/tests/helpers/stash-published-version.ts b/ts/tests/helpers/stash-published-version.ts index 076ad34dd..153fac8f7 100644 --- a/ts/tests/helpers/stash-published-version.ts +++ b/ts/tests/helpers/stash-published-version.ts @@ -2,6 +2,19 @@ import fs from 'fs-extra'; const PACKAGE_PATH = 'node_modules/ember-cli-typescript'; +function isNodeError(e: unknown): e is NodeJS.ErrnoException { + // Not full-proof but good enough for our purposes. + return e instanceof Error && 'code' in e; +} + +function handleError(e: unknown) { + // Ignore the case where we just don't have the files to move. (And hope this + // works correctly?) + if (!isNodeError(e) || e.code !== 'ENOENT') { + throw e; + } +} + /** * We have assorted devDependencies that themselves depend on `ember-cli-typescript`. * This means we have a published copy of the addon present in `node_modules` normally, @@ -12,10 +25,19 @@ const PACKAGE_PATH = 'node_modules/ember-cli-typescript'; */ export function setupPublishedVersionStashing(hooks: Mocha.Suite): void { hooks.beforeAll(async () => { - await fs.move(PACKAGE_PATH, `${PACKAGE_PATH}.published`); + fs.move(PACKAGE_PATH, `${PACKAGE_PATH}.published`).catch(); + try { + await fs.move(PACKAGE_PATH, `${PACKAGE_PATH}.published`); + } catch (e: unknown) { + handleError(e); + } }); hooks.afterAll(async () => { - await fs.move(`${PACKAGE_PATH}.published`, PACKAGE_PATH); + try { + await fs.move(`${PACKAGE_PATH}.published`, PACKAGE_PATH); + } catch (e) { + handleError(e); + } }); } From a9e3fffb1014999d1d86614564c937234b3f57be Mon Sep 17 00:00:00 2001 From: Chris Krycho Date: Thu, 3 Mar 2022 16:17:52 -0700 Subject: [PATCH 6/6] Account for Windows' ridiculous carriage return --- ts/tests/commands/precompile-test.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/ts/tests/commands/precompile-test.ts b/ts/tests/commands/precompile-test.ts index 0094528ae..039eac0fa 100644 --- a/ts/tests/commands/precompile-test.ts +++ b/ts/tests/commands/precompile-test.ts @@ -1,9 +1,11 @@ import * as fs from 'fs-extra'; import * as path from 'path'; +import { EOL } from 'os'; import { hook } from 'capture-console'; import ember from 'ember-cli-blueprint-test-helpers/lib/helpers/ember'; import blueprintHelpers from 'ember-cli-blueprint-test-helpers/helpers'; import ts from 'typescript'; + const setupTestHooks = blueprintHelpers.setupTestHooks; const emberNew = blueprintHelpers.emberNew; @@ -28,7 +30,7 @@ describe('Acceptance: ts:precompile command', function () { let declaration = file('test-file.d.ts'); expect(declaration).to.exist; expect(declaration.content.trim()).to.equal( - `export declare const testString: string;\n//# sourceMappingURL=test-file.d.ts.map` + `export declare const testString: string;${EOL}//# sourceMappingURL=test-file.d.ts.map` ); }); @@ -76,7 +78,7 @@ describe('Acceptance: ts:precompile command', function () { let declaration = file('src/test-file.d.ts'); expect(declaration).to.exist; expect(declaration.content.trim()).to.equal( - `export declare const testString: string;\n//# sourceMappingURL=test-file.d.ts.map` + `export declare const testString: string;${EOL}//# sourceMappingURL=test-file.d.ts.map` ); }); }); @@ -100,7 +102,7 @@ describe('Acceptance: ts:precompile command', function () { let declaration = file('test-file.d.ts'); expect(declaration).to.exist; expect(declaration.content.trim()).to.equal( - `export declare const testString: string;\n//# sourceMappingURL=test-file.d.ts.map` + `export declare const testString: string;${EOL}//# sourceMappingURL=test-file.d.ts.map` ); }); }); @@ -128,7 +130,7 @@ describe('Acceptance: ts:precompile command', function () { const declaration = file('test-support/test-file.d.ts'); expect(declaration).to.exist; expect(declaration.content.trim()).to.equal( - `export declare const testString: string;\n//# sourceMappingURL=test-file.d.ts.map` + `export declare const testString: string;${EOL}//# sourceMappingURL=test-file.d.ts.map` ); }); }); @@ -164,13 +166,13 @@ describe('Acceptance: ts:precompile command', function () { const componentDecl = file('components/my-component.d.ts'); expect(componentDecl).to.exist; expect(componentDecl.content.trim()).to.equal( - `export declare const testString: string;\n//# sourceMappingURL=my-component.d.ts.map` + `export declare const testString: string;${EOL}//# sourceMappingURL=my-component.d.ts.map` ); const testSupportDecl = file('test-support/test-file.d.ts'); expect(testSupportDecl).to.exist; expect(testSupportDecl.content.trim()).to.equal( - `export declare const anotherTestString: string;\n//# sourceMappingURL=test-file.d.ts.map` + `export declare const anotherTestString: string;${EOL}//# sourceMappingURL=test-file.d.ts.map` ); }); });