From 3ad693fb6614b729938f96a0acf2c8afb5e63e21 Mon Sep 17 00:00:00 2001 From: Will Temple Date: Thu, 17 Feb 2022 16:43:46 -0500 Subject: [PATCH] [dev-tool] Implement `esModuleInterop` for sample transpilation (#20391) * [dev-tool] Emulate esModuleInterop * Regenerate expectations * Correctly handle ambient .d.ts files --- .../dev-tool/src/commands/samples/index.ts | 1 - .../tsToJs.ts => util/samples/convert.ts} | 34 +------------- .../dev-tool/src/util/samples/generation.ts | 44 +++++++++++++------ .../dev-tool/src/util/samples/processor.ts | 9 ++-- .../tools/dev-tool/src/util/samples/syntax.ts | 16 ------- .../dev-tool/src/util/samples/transforms.ts | 27 +++++++++--- .../cjs-forms/javascript/index.js | 7 +++ .../cjs-forms/javascript/package.json | 4 +- .../cjs-forms/typescript/package.json | 4 +- .../cjs-forms/typescript/src/ambient.d.ts | 18 ++++++++ .../cjs-forms/typescript/src/index.ts | 5 +++ .../files/inputs/cjs-forms/ambient.d.ts | 18 ++++++++ .../files/inputs/cjs-forms/config.json | 4 ++ .../samples/files/inputs/cjs-forms/index.ts | 5 +++ 14 files changed, 122 insertions(+), 74 deletions(-) rename common/tools/dev-tool/src/{commands/samples/tsToJs.ts => util/samples/convert.ts} (79%) create mode 100644 common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/src/ambient.d.ts create mode 100644 common/tools/dev-tool/test/samples/files/inputs/cjs-forms/ambient.d.ts diff --git a/common/tools/dev-tool/src/commands/samples/index.ts b/common/tools/dev-tool/src/commands/samples/index.ts index 756b346df2bd..9e53b36f3f59 100644 --- a/common/tools/dev-tool/src/commands/samples/index.ts +++ b/common/tools/dev-tool/src/commands/samples/index.ts @@ -10,6 +10,5 @@ export default subCommand(commandInfo, { prep: () => import("./prep"), publish: () => import("./publish"), run: () => import("./run"), - "ts-to-js": () => import("./tsToJs"), "check-node-versions": () => import("./checkNodeVersions"), }); diff --git a/common/tools/dev-tool/src/commands/samples/tsToJs.ts b/common/tools/dev-tool/src/util/samples/convert.ts similarity index 79% rename from common/tools/dev-tool/src/commands/samples/tsToJs.ts rename to common/tools/dev-tool/src/util/samples/convert.ts index c493d25f03a2..e3833e09887b 100644 --- a/common/tools/dev-tool/src/commands/samples/tsToJs.ts +++ b/common/tools/dev-tool/src/util/samples/convert.ts @@ -1,25 +1,15 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. -import fs from "fs-extra"; -import path from "path"; import { EOL } from "os"; import * as prettier from "prettier"; import ts from "typescript"; -import { leafCommand, makeCommandInfo } from "../../framework/command"; - -import { createPrinter } from "../../util/printer"; -import { toCommonJs } from "../../util/samples/transforms"; +import { createPrinter } from "../printer"; const log = createPrinter("ts-to-js"); -export const commandInfo = makeCommandInfo( - "ts-to-js", - "convert a TypeScript sample to a JavaScript equivalent using our conventions for samples" -); - const prettierOptions: prettier.Options = { // eslint-disable-next-line @typescript-eslint/no-var-requires ...(require("../../../../eslint-plugin-azure-sdk/prettier.json") as prettier.Options), @@ -119,25 +109,3 @@ export function convert(srcText: string, transpileOptions?: ts.TranspileOptions) return postTransform(output.outputText); } - -export default leafCommand(commandInfo, async (options) => { - if (options.args.length !== 2) { - throw new Error("Wrong number of arguments. Got " + options.args.length + " but expected 2."); - } - - const [src, dest] = options.args.map(path.normalize); - - const srcText = (await fs.readFile(src)).toString("utf-8"); - - const outputText = convert(srcText, { - fileName: src, - transformers: { - after: [toCommonJs], - }, - }); - - await fs.ensureDir(path.dirname(dest)); - await fs.writeFile(dest, outputText); - - return true; -}); diff --git a/common/tools/dev-tool/src/util/samples/generation.ts b/common/tools/dev-tool/src/util/samples/generation.ts index 112e1216c40a..3114b515a8e9 100644 --- a/common/tools/dev-tool/src/util/samples/generation.ts +++ b/common/tools/dev-tool/src/util/samples/generation.ts @@ -87,7 +87,7 @@ export async function makeSampleGenerationInfo( onError: () => void ): Promise { const sampleSources = await collect( - findMatchingFiles(sampleSourcesPath, (name) => name.endsWith(".ts")) + findMatchingFiles(sampleSourcesPath, (name) => name.endsWith(".ts") && !name.endsWith(".d.ts")) ); const sampleConfiguration = getSampleConfiguration(projectInfo.packageJson); @@ -103,7 +103,19 @@ export async function makeSampleGenerationInfo( return undefined as never; } - const moduleInfos = await processSources(sampleSourcesPath, sampleSources, fail); + const requireInScope = (moduleSpecifier: string) => { + try { + return require(path.join( + projectInfo.path, + "node_modules", + moduleSpecifier.split("/").join(path.sep) + )); + } catch { + return require(moduleSpecifier); + } + }; + + const moduleInfos = await processSources(sampleSourcesPath, sampleSources, fail, requireInScope); const defaultDependencies: Record = { // If we are a beta package, use "next", otherwise we will use "latest" @@ -279,14 +291,18 @@ export async function makeSamplesFactory( log.debug("Computed full generation path:", versionFolder); - const info = await makeSampleGenerationInfo( - projectInfo, - sourcePath ?? path.join(projectInfo.path, DEV_SAMPLES_BASE), - versionFolder, - onError - ); + const finalSourcePath = sourcePath ?? path.join(projectInfo.path, DEV_SAMPLES_BASE); + + const info = await makeSampleGenerationInfo(projectInfo, finalSourcePath, versionFolder, onError); info.isBeta = isBeta; + // Ambient declarations ().d.ts files) are excluded from the compile graph in the transpiler. We will still copy them + // into typescript/src so that they will be availabled for transpilation. + const dtsFiles: Array<[string, string]> = []; + for await (const name of findMatchingFiles(finalSourcePath, (name) => name.endsWith(".d.ts"))) { + dtsFiles.push([path.relative(finalSourcePath, name), name]); + } + if (hadError) { throw new Error("Instantiation of sample metadata information failed with errors."); } @@ -335,12 +351,14 @@ export async function makeSamplesFactory( file("tsconfig.json", () => jsonify(DEFAULT_TYPESCRIPT_CONFIG)), copy("sample.env", path.join(projectInfo.path, "sample.env")), // We copy the samples sources in to the `src` folder on the typescript side - dir( - "src", - info.moduleInfos.map(({ relativeSourcePath, filePath }) => + dir("src", [ + ...info.moduleInfos.map(({ relativeSourcePath, filePath }) => file(relativeSourcePath, () => postProcess(fs.readFileSync(filePath))) - ) - ), + ), + ...dtsFiles.map(([relative, absolute]) => + file(relative, fs.readFileSync(absolute)) + ), + ]), ]), dir("javascript", [ file("README.md", () => diff --git a/common/tools/dev-tool/src/util/samples/processor.ts b/common/tools/dev-tool/src/util/samples/processor.ts index 706417d62dad..28a638171609 100644 --- a/common/tools/dev-tool/src/util/samples/processor.ts +++ b/common/tools/dev-tool/src/util/samples/processor.ts @@ -4,20 +4,21 @@ import fs from "fs-extra"; import path from "path"; import * as ts from "typescript"; -import { convert } from "../../commands/samples/tsToJs"; +import { convert } from "./convert"; import { createPrinter } from "../printer"; import { createAccumulator } from "../typescript/accumulator"; import { createDiagnosticEmitter } from "../typescript/diagnostic"; import { AzSdkMetaTags, AZSDK_META_TAG_PREFIX, ModuleInfo, VALID_AZSDK_META_TAGS } from "./info"; import { testSyntax } from "./syntax"; -import { isDependency, isRelativePath, toCommonJs } from "./transforms"; +import { createToCommonJsTransform, isDependency, isRelativePath } from "./transforms"; const log = createPrinter("samples:processor"); export async function processSources( sourceDirectory: string, sources: string[], - fail: (...values: unknown[]) => never + fail: (...values: unknown[]) => never, + requireInScope: (moduleSpecifier: string) => unknown ): Promise { // Project-scoped information (shared between all source files) let hadUnsupportedSyntax = false; @@ -105,7 +106,7 @@ export async function processSources( fileName: source, transformers: { before: [sourceProcessor], - after: [toCommonJs], + after: [createToCommonJsTransform(requireInScope)], }, }); diff --git a/common/tools/dev-tool/src/util/samples/syntax.ts b/common/tools/dev-tool/src/util/samples/syntax.ts index b3817151b6e2..f07c6a82b57b 100644 --- a/common/tools/dev-tool/src/util/samples/syntax.ts +++ b/common/tools/dev-tool/src/util/samples/syntax.ts @@ -3,7 +3,6 @@ import * as ts from "typescript"; import { createPrinter } from "../printer"; -import { isNodeBuiltin, isRelativePath } from "./transforms"; const log = createPrinter("samples:syntax"); @@ -47,21 +46,6 @@ const SYNTAX_VIABILITY_TESTS = { // import("foo") ImportExpression: (node: ts.Node) => ts.isCallExpression(node) && node.expression.kind === ts.SyntaxKind.ImportKeyword, - // This can't be supported without going to great lengths to emulate esModuleInterop behavior. - // It's a little more involved to test for. We only care about `import from ` - // where does not refer to a builtin or a relative module path. - ExternalDefaultImport: (node: ts.Node) => { - const isDefaultImport = - ts.isImportDeclaration(node) && - node.importClause?.name && - ts.isIdentifier(node.importClause.name); - - if (!isDefaultImport) return false; - - const { text: moduleSpecifier } = node.moduleSpecifier as ts.StringLiteralLike; - - return isDefaultImport && !isNodeBuiltin(moduleSpecifier) && !isRelativePath(moduleSpecifier); - }, }, // Supported in Node 14+ ES2020: { diff --git a/common/tools/dev-tool/src/util/samples/transforms.ts b/common/tools/dev-tool/src/util/samples/transforms.ts index da87795e375d..ce040eeffab7 100644 --- a/common/tools/dev-tool/src/util/samples/transforms.ts +++ b/common/tools/dev-tool/src/util/samples/transforms.ts @@ -16,10 +16,15 @@ import nodeBuiltins from "builtin-modules"; * @param context - the compiler API context * @returns a visitor that performs the transform to CommonJS */ -export const toCommonJs: ts.TransformerFactory = (context) => (sourceFile) => { +export const createToCommonJsTransform: ( + getPackage: (moduleSpecifier: string) => unknown +) => ts.TransformerFactory = (getPackage) => (context) => (sourceFile) => { const visitor: ts.Visitor = (node) => { if (ts.isImportDeclaration(node)) { - return ts.visitNode(importDeclarationToCommonJs(node, context.factory, sourceFile), visitor); + return ts.visitNode( + importDeclarationToCommonJs(node, getPackage, context.factory, sourceFile), + visitor + ); } else if (ts.isExportDeclaration(node) || ts.isExportAssignment(node)) { // TypeScript can choose to emit `export {}` in some cases, so we will remove any export declarations. return context.factory.createEmptyStatement(); @@ -31,6 +36,10 @@ export const toCommonJs: ts.TransformerFactory = (context) => (so return ts.visitNode(sourceFile, visitor); }; +interface TranspiledModule { + __esModule?: boolean; +} + /** * Convert an ImportDeclaration into a require call. * @@ -54,6 +63,7 @@ export const toCommonJs: ts.TransformerFactory = (context) => (so */ export function importDeclarationToCommonJs( decl: ts.ImportDeclaration, + requireInScope: (moduleSpecifier: string) => unknown, nodeFactory?: ts.NodeFactory, sourceFile?: ts.SourceFile ): ts.Statement { @@ -93,10 +103,17 @@ export function importDeclarationToCommonJs( const isDefaultImport = ts.isIdentifier(primaryBinding) && - // We only allow default imports on relative modules and node builtins, but on node builtins they are actually - // just namespace imports in disguise. This is because of esModuleInterop compatibility in our tsconfig.json. + // Node builtins are never treated as default imports. !isNodeBuiltin(moduleSpecifierText) && - (!namedBindings || !ts.isNamespaceImport(namedBindings)); + // If this is a namespace import, then it's not a default import. + !(namedBindings && ts.isNamespaceImport(namedBindings)) && + // @azure imports are treated as defaults + (/^@azure(-[a-z0-9]*)?\//.test(moduleSpecifierText) || + // Relative imports are treated as defaults + isRelativePath(moduleSpecifierText) || + // Ultimately, if the module has an `__esModule` field, we treat it as a default import. This mimics the behavior + // of runtime `esModuleInterop` + (requireInScope(moduleSpecifierText) as TranspiledModule).__esModule); // The declaration will usually only contain one item, and it will be something like: // diff --git a/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/javascript/index.js b/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/javascript/index.js index b362a8d5f6da..82bcd7f3c58c 100644 --- a/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/javascript/index.js +++ b/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/javascript/index.js @@ -24,6 +24,13 @@ require("./hasSideEffects"); const path_1 = require("path"); const path_2 = require("path"); +const test1 = require("@azure/test1").default, + { x: x1 } = require("@azure/test1"); +const test2 = require("@azure-test2/test2").default, + { x: x2 } = require("@azure-test2/test2"); + +void [test1, test2, x1, x2]; + async function main() { const waitTime = process.env.WAIT_TIME || "5000"; const delayMs = parseInt(waitTime); diff --git a/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/javascript/package.json b/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/javascript/package.json index 71f484102a8a..a8244ffb3b8f 100644 --- a/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/javascript/package.json +++ b/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/javascript/package.json @@ -22,6 +22,8 @@ "homepage": "https://github.com/Azure/azure-sdk-for-js/tree/main/common/tools/dev-tool/test/samples/files/expectations/cjs-forms", "dependencies": { "cjs-forms": "latest", - "dotenv": "latest" + "dotenv": "latest", + "@azure/test1": "latest", + "@azure-test2/test2": "next" } } diff --git a/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/package.json b/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/package.json index d2761bfb2a4c..b125b58a14c4 100644 --- a/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/package.json +++ b/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/package.json @@ -26,7 +26,9 @@ "homepage": "https://github.com/Azure/azure-sdk-for-js/tree/main/common/tools/dev-tool/test/samples/files/expectations/cjs-forms", "dependencies": { "cjs-forms": "latest", - "dotenv": "latest" + "dotenv": "latest", + "@azure/test1": "latest", + "@azure-test2/test2": "next" }, "devDependencies": { "@types/node": "^12.0.0", diff --git a/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/src/ambient.d.ts b/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/src/ambient.d.ts new file mode 100644 index 000000000000..b743b9bc83d2 --- /dev/null +++ b/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/src/ambient.d.ts @@ -0,0 +1,18 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +/** + * These modules are declared to help us type-check the examples, which have special cases for @azure packages. + */ + +declare module "@azure/test1" { + declare const x: unknown; + export default x; + export { x }; +} + +declare module "@azure-test2/test2" { + declare const x: unknown; + export default x; + export { x }; +} diff --git a/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/src/index.ts b/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/src/index.ts index 13f88f7cc443..1f927c806862 100644 --- a/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/src/index.ts +++ b/common/tools/dev-tool/test/samples/files/expectations/cjs-forms/typescript/src/index.ts @@ -23,6 +23,11 @@ import "./hasSideEffects"; import * as path_1 from "path"; import path_2 from "path"; +import test1, { x as x1 } from "@azure/test1"; +import test2, { x as x2 } from "@azure-test2/test2"; + +void [test1, test2, x1, x2]; + async function main() { const waitTime = process.env.WAIT_TIME || "5000"; const delayMs = parseInt(waitTime); diff --git a/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/ambient.d.ts b/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/ambient.d.ts new file mode 100644 index 000000000000..b743b9bc83d2 --- /dev/null +++ b/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/ambient.d.ts @@ -0,0 +1,18 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +/** + * These modules are declared to help us type-check the examples, which have special cases for @azure packages. + */ + +declare module "@azure/test1" { + declare const x: unknown; + export default x; + export { x }; +} + +declare module "@azure-test2/test2" { + declare const x: unknown; + export default x; + export { x }; +} diff --git a/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/config.json b/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/config.json index d5f65645df0e..83d524a8e090 100644 --- a/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/config.json +++ b/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/config.json @@ -6,5 +6,9 @@ "apiRefLink": "https://docs.microsoft.com/", "requiredResources": { "test": "https://contoso.com" + }, + "dependencyOverrides": { + "@azure/test1": "latest", + "@azure-test2/test2": "next" } } diff --git a/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/index.ts b/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/index.ts index 13f88f7cc443..1f927c806862 100644 --- a/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/index.ts +++ b/common/tools/dev-tool/test/samples/files/inputs/cjs-forms/index.ts @@ -23,6 +23,11 @@ import "./hasSideEffects"; import * as path_1 from "path"; import path_2 from "path"; +import test1, { x as x1 } from "@azure/test1"; +import test2, { x as x2 } from "@azure-test2/test2"; + +void [test1, test2, x1, x2]; + async function main() { const waitTime = process.env.WAIT_TIME || "5000"; const delayMs = parseInt(waitTime);