From 3a12ba514abea673a91d3d704c7ddf613905c156 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 18 Oct 2022 12:46:47 +0200 Subject: [PATCH] ref(build): Remove `constToVarPlugin` (#5970) Co-authored-by: Abhijeet Prasad --- jest/transformers/constReplacer.ts | 84 ------------------- packages/browser/src/integrations/trycatch.ts | 4 +- packages/nextjs/src/performance/client.ts | 8 +- rollup/npmHelpers.js | 3 - rollup/plugins/npmPlugins.js | 26 ------ scripts/test.ts | 39 --------- 6 files changed, 6 insertions(+), 158 deletions(-) delete mode 100644 jest/transformers/constReplacer.ts diff --git a/jest/transformers/constReplacer.ts b/jest/transformers/constReplacer.ts deleted file mode 100644 index 725aa9ac48c8..000000000000 --- a/jest/transformers/constReplacer.ts +++ /dev/null @@ -1,84 +0,0 @@ -/** - * This is a transformer which `ts-jest` applies during the compilation process, which switches all of the `const`s out - * for `var`s. Unlike in our package builds, where we do the same substiution for bundle size reasons, here we do it - * because otherwise `const global = getGlobalObject()` throws an error about redifining `global`. (This didn't used to - * be a problem because our down-compilation did the `const`-`var` substitution for us, but now that we're ES6-only, we - * have to do it ourselves.) - * - * Note: If you ever have to change this, and are testing it locally in the process, be sure to call - * `yarn jest --clearCache` - * before each test run, as transformation results are cached between runs. - */ - -import { - createVariableDeclarationList, - getCombinedNodeFlags, - isVariableDeclarationList, - Node, - NodeFlags, - SourceFile, - TransformationContext, - Transformer, - TransformerFactory, - visitEachChild, - visitNode, - VisitResult, -} from 'typescript'; - -// These can be anything - they're just used to construct a cache key for the transformer returned by the factory below. -// This really only matters when you're testing the transformer itself, as changing these values gives you a quick way -// to invalidate the cache and ensure that changes you've made to the code here are immediately picked up on and used. -export const name = 'const-to-var'; -export const version = '1.0'; - -/** - * Check whether the given AST node represents a `const` token. - * - * This function comes from the TS compiler, and is copied here to get around the fact that it's not exported by the - * `typescript` package. - * - * @param node The node to check - * @returns A boolean indicating if the node is a `const` token. - */ -function isVarConst(node: Node): boolean { - // eslint-disable-next-line no-bitwise - return !!(getCombinedNodeFlags(node) & NodeFlags.Const); -} - -/** - * Return a set of nested factory functions, which ultimately creates an AST-node visitor function, which can modify - * each visited node as it sees fit, and uses it to walk the AST, returning the results. - * - * In our case, we're modifying all `const` variable declarations to use `var` instead. - */ -export function factory(): TransformerFactory { - // Create the transformer - function transformerFactory(context: TransformationContext): Transformer { - // Create a visitor function and use it to walk the AST - function transformer(sourceFile: SourceFile): SourceFile { - // This visitor function can either return a node, in which case the subtree rooted at the returned node is - // substituted for the subtree rooted at the visited node, or can use the recursive `visitEachChild` function - // provided by TS to continue traversing the tree. - function visitor(node: Node): VisitResult { - // If we've found a `const` declaration, return a `var` declaration in its place - if (isVariableDeclarationList(node) && isVarConst(node)) { - // A declaration list with a `None` flag defaults to using `var` - return createVariableDeclarationList(node.declarations, NodeFlags.None); - } - - // This wasn't a node we're interested in, so keep walking down the tree. - return visitEachChild(node, visitor, context); - } - - // Having defined our visitor, pass it to the TS-provided `visitNode` function, which will use it to walk the AST, - // and return the results of that walk. - return visitNode(sourceFile, visitor); - } - - // Back in the transformer factory, return the transformer we just created - return transformer; - } - - // Finally, we're back in `factory`, and can return the whole nested system - return transformerFactory; -} diff --git a/packages/browser/src/integrations/trycatch.ts b/packages/browser/src/integrations/trycatch.ts index af06c54d6290..b1ef94a89e1d 100644 --- a/packages/browser/src/integrations/trycatch.ts +++ b/packages/browser/src/integrations/trycatch.ts @@ -183,9 +183,9 @@ function _wrapXHR(originalSend: () => void): () => void { /** JSDoc */ function _wrapEventTarget(target: string): void { // eslint-disable-next-line @typescript-eslint/no-explicit-any - const global = WINDOW as { [key: string]: any }; + const globalObject = WINDOW as { [key: string]: any }; // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - const proto = global[target] && global[target].prototype; + const proto = globalObject[target] && globalObject[target].prototype; // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, no-prototype-builtins if (!proto || !proto.hasOwnProperty || !proto.hasOwnProperty('addEventListener')) { diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index 0702a35f8f67..6c918644f940 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -11,7 +11,7 @@ import type { NEXT_DATA as NextData } from 'next/dist/next-server/lib/utils'; import { default as Router } from 'next/router'; import type { ParsedUrlQuery } from 'querystring'; -const global = WINDOW as typeof WINDOW & { +const globalObject = WINDOW as typeof WINDOW & { __BUILD_MANIFEST?: { sortedPages?: string[]; }; @@ -57,7 +57,7 @@ function extractNextDataTagInformation(): NextDataTagInfo { let nextData: SentryEnhancedNextData | undefined; // Let's be on the safe side and actually check first if there is really a __NEXT_DATA__ script tag on the page. // Theoretically this should always be the case though. - const nextDataTag = global.document.getElementById('__NEXT_DATA__'); + const nextDataTag = globalObject.document.getElementById('__NEXT_DATA__'); if (nextDataTag && nextDataTag.innerHTML) { try { nextData = JSON.parse(nextDataTag.innerHTML); @@ -122,7 +122,7 @@ export function nextRouterInstrumentation( startTransactionOnLocationChange: boolean = true, ): void { const { route, traceParentData, baggage, params } = extractNextDataTagInformation(); - prevLocationName = route || global.location.pathname; + prevLocationName = route || globalObject.location.pathname; if (startTransactionOnPageLoad) { const source = route ? 'route' : 'url'; @@ -197,7 +197,7 @@ export function nextRouterInstrumentation( } function getNextRouteFromPathname(pathname: string): string | undefined { - const pageRoutes = (global.__BUILD_MANIFEST || {}).sortedPages; + const pageRoutes = (globalObject.__BUILD_MANIFEST || {}).sortedPages; // Page route should in 99.999% of the cases be defined by now but just to be sure we make a check here if (!pageRoutes) { diff --git a/rollup/npmHelpers.js b/rollup/npmHelpers.js index 2a5724a9363c..b7d0fd187e14 100644 --- a/rollup/npmHelpers.js +++ b/rollup/npmHelpers.js @@ -8,7 +8,6 @@ import * as path from 'path'; import deepMerge from 'deepmerge'; import { - makeConstToVarPlugin, makeExtractPolyfillsPlugin, makeNodeResolvePlugin, makeCleanupPlugin, @@ -30,7 +29,6 @@ export function makeBaseNPMConfig(options = {}) { const nodeResolvePlugin = makeNodeResolvePlugin(); const sucrasePlugin = makeSucrasePlugin(); const debugBuildStatementReplacePlugin = makeDebugBuildStatementReplacePlugin(); - const constToVarPlugin = makeConstToVarPlugin(); const cleanupPlugin = makeCleanupPlugin(); const extractPolyfillsPlugin = makeExtractPolyfillsPlugin(); @@ -81,7 +79,6 @@ export function makeBaseNPMConfig(options = {}) { nodeResolvePlugin, sucrasePlugin, debugBuildStatementReplacePlugin, - constToVarPlugin, cleanupPlugin, extractPolyfillsPlugin, ], diff --git a/rollup/plugins/npmPlugins.js b/rollup/plugins/npmPlugins.js index 27e5d96ae59c..ec162615623f 100644 --- a/rollup/plugins/npmPlugins.js +++ b/rollup/plugins/npmPlugins.js @@ -22,32 +22,6 @@ export function makeSucrasePlugin() { }); } -/** - * Create a plugin to switch all instances of `const` to `var`, both to prevent problems when we shadow `global` and - * because it's fewer characters. - * - * Note that the generated plugin runs the replacement both before and after rollup does its code manipulation, to - * increase the chances that nothing is missed. - * - * TODO This is pretty brute-force-y. Perhaps we could switch to using a parser, the way we (will) do for both our jest - * transformer and the polyfill build script. - * - * @returns An instance of the `@rollup/plugin-replace` plugin - */ -export function makeConstToVarPlugin() { - return replace({ - // TODO `preventAssignment` will default to true in version 5.x of the replace plugin, at which point we can get rid - // of this. (It actually makes no difference in this case whether it's true or false, since we never assign to - // `const`, but if we don't give it a value, it will spam with warnings.) - preventAssignment: true, - values: { - // Include a space at the end to guarantee we're not accidentally catching the beginning of the words "constant," - // "constantly," etc. - 'const ': 'var ', - }, - }); -} - /** * Create a plugin which can be used to pause the build process at the given hook. * diff --git a/scripts/test.ts b/scripts/test.ts index a4904abe50f6..0ed419da8ea7 100644 --- a/scripts/test.ts +++ b/scripts/test.ts @@ -1,6 +1,5 @@ import * as childProcess from 'child_process'; import * as fs from 'fs'; -import * as path from 'path'; const CURRENT_NODE_VERSION = process.version.replace('v', '').split('.')[0]; @@ -62,42 +61,6 @@ function installLegacyDeps(legacyDeps: string[] = []): void { run(`yarn add --dev --ignore-engines --ignore-scripts --ignore-workspace-root-check ${legacyDeps.join(' ')}`); } -/** - * Add a tranformer to our jest config, to do the same `const`-to-`var` replacement as our rollup plugin does. - * - * This is needed because Node 8 doesn't like the way we shadow `global` (`const global = getGlobalObject()`). Changing - * it to a `var` solves this by making it redeclarable. - * - */ -function addJestTransformer(): void { - // Though newer `ts-jest` versions support transformers written in TS, the legacy version does not. - run('yarn tsc --skipLibCheck jest/transformers/constReplacer.ts'); - - // Loading the existing Jest config will error out unless the config file has an accompanying types file, so we have - // to create that before we can load it. - run('yarn tsc --allowJs --skipLibCheck --declaration --emitDeclarationOnly jest/jest.config.js'); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const jestConfig = require('../jest/jest.config.js'); - - // Inject the transformer - jestConfig.globals['ts-jest'].astTransformers = ['/../../jest/transformers/constReplacer.js']; - - // When we required the jest config file above, all expressions it contained were evaluated. Specifically, the - // `rootDir: process.cwd()` - // entry was replaced with - // `rootDir: ""`, - // Though it's a little brute-force-y, the easiest way to fix this is to just stringify the code and perform the - // substitution in reverse. - const stringifiedConfig = JSON.stringify(jestConfig, null, 2).replace( - `"rootDir": "${process.cwd()}"`, - 'rootDir: process.cwd()', - ); - - // Now we just have to convert it back to a module and write it to disk - const code = `module.exports = ${stringifiedConfig}`; - fs.writeFileSync(path.resolve('jest/jest.config.js'), code); -} - /** * Modify a json file on disk. * @@ -151,8 +114,6 @@ function runWithIgnores(skipPackages: string[] = []): void { function runTests(): void { if (CURRENT_NODE_VERSION === '8') { installLegacyDeps(NODE_8_LEGACY_DEPENDENCIES); - // Inject a `const`-to-`var` transformer, in order to stop Node 8 from complaining when we shadow `global` - addJestTransformer(); // TODO Right now, this just skips incompatible tests, but it could be skipping more (hence the aspirational name), // and not just in Node 8. See `skipNonNodeTests`'s docstring. skipNonNodeTests();