From 51cdcfa58113efdaa3c22aafe48f14c9d62e3a9e Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Wed, 9 Jun 2021 13:39:13 -0600 Subject: [PATCH 1/2] Fix Placeholder cache solution when used with requireFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ensure we include the package’s version in the configuration, so that the babel-loader can detect module version changes in it’s cache key consideration. Without this, installing a new node_module may not result in babel-loaders cache expiration occurring. --- packages/core/package.json | 2 +- packages/core/src/app.ts | 7 ++++++- packages/core/src/portable.ts | 21 +++++++++++++++++++ packages/core/tests/portable.test.ts | 14 +++++++++++++ test-packages/macro-sample-addon/package.json | 5 ++--- yarn.lock | 7 +++++++ 6 files changed, 51 insertions(+), 5 deletions(-) create mode 100644 packages/core/tests/portable.test.ts diff --git a/packages/core/package.json b/packages/core/package.json index e7e35af9a..d50dae34f 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -57,7 +57,7 @@ "lodash": "^4.17.10", "pkg-up": "^3.1.0", "resolve": "^1.8.1", - "resolve-package-path": "^1.2.2", + "resolve-package-path": "^4.0.1", "strip-bom": "^3.0.0", "typescript-memoize": "^1.0.0", "walk-sync": "^1.1.3", diff --git a/packages/core/src/app.ts b/packages/core/src/app.ts index 49da8f39f..810ac8a16 100644 --- a/packages/core/src/app.ts +++ b/packages/core/src/app.ts @@ -922,7 +922,12 @@ export class AppBuilder { } cursor = resolve.sync(target, { basedir: dirname(cursor) }); } - return { requireFile: cursor, useMethod: hint.useMethod }; + + return { + requireFile: cursor, + useMethod: hint.useMethod, + packageVersion: readJSONSync(cursor).version, + }; }); } diff --git a/packages/core/src/portable.ts b/packages/core/src/portable.ts index 611c2f151..d4bce561b 100644 --- a/packages/core/src/portable.ts +++ b/packages/core/src/portable.ts @@ -1,6 +1,8 @@ import mapValues from 'lodash/mapValues'; import assertNever from 'assert-never'; import { Memoize } from 'typescript-memoize'; +import resolvePackagePath from 'resolve-package-path'; +import { readJSONSync } from 'fs-extra'; export const protocol = '__embroider_portable_values__'; const { globalValues, nonce } = setupGlobals(); @@ -13,9 +15,23 @@ export interface PortableResult { export interface PortableHint { requireFile: string; + packageVersion: string | undefined; useMethod?: string; } +const { findUpPackagePath } = resolvePackagePath; + +export function maybeNodeModuleVersion(path: string) { + const packagePath = findUpPackagePath(path); + + if (packagePath === null) { + // no package was found + return undefined; // should this bust the cache or ... ? + } else { + return readJSONSync(packagePath).version; + } +} + export class Portable { constructor( private opts: { @@ -83,6 +99,7 @@ export class Portable { embroiderPlaceholder: true, type: 'broccoli-parallel', requireFile: found.requireFile, + packageVersion: maybeNodeModuleVersion(found.requireFile), useMethod: found.useMethod, }, isParallelSafe: true, @@ -154,6 +171,7 @@ interface BroccoliParallelPlaceholder { embroiderPlaceholder: true; type: 'broccoli-parallel'; requireFile: string; + packageVersion: string | undefined; useMethod: string | undefined; buildUsing: string | undefined; params: any; @@ -162,6 +180,7 @@ interface BroccoliParallelPlaceholder { interface HTMLBarsParallelPlaceholder { embroiderPlaceholder: true; type: 'htmlbars-parallel'; + packageVersion: string | undefined; requireFile: string; buildUsing: string; params: any; @@ -193,6 +212,7 @@ function maybeBroccoli(object: any): BroccoliParallelPlaceholder | undefined { embroiderPlaceholder: true, type: 'broccoli-parallel', requireFile: object._parallelBabel.requireFile, + packageVersion: maybeNodeModuleVersion(object._parallelBabel.requireFile), buildUsing: object._parallelBabel.buildUsing, useMethod: object._parallelBabel.useMethod, params: object._parallelBabel.params, @@ -238,6 +258,7 @@ function maybeHTMLBars(object: any): HTMLBarsParallelPlaceholder | undefined { embroiderPlaceholder: true, type: 'htmlbars-parallel', requireFile: object.parallelBabel.requireFile, + packageVersion: maybeNodeModuleVersion(object.parallelBabel.requireFile), buildUsing: String(object.parallelBabel.buildUsing), params: object.parallelBabel.params, }; diff --git a/packages/core/tests/portable.test.ts b/packages/core/tests/portable.test.ts new file mode 100644 index 000000000..813dc555c --- /dev/null +++ b/packages/core/tests/portable.test.ts @@ -0,0 +1,14 @@ +import { maybeNodeModuleVersion } from '../src/portable'; +import { readJSONSync } from 'fs-extra'; + +const EMBROIDER_CORE_VERSION = readJSONSync('../../package.json').version; + +describe('maybeNodeModuleVersion', () => { + test('it', () => { + expect(maybeNodeModuleVersion('/dev/null')).toEqual(undefined); + expect(maybeNodeModuleVersion('/does/not/exist')).toEqual(undefined); + expect(maybeNodeModuleVersion(__dirname)).toEqual(EMBROIDER_CORE_VERSION); + expect(maybeNodeModuleVersion(__filename)).toEqual(EMBROIDER_CORE_VERSION); + }); +}); + diff --git a/test-packages/macro-sample-addon/package.json b/test-packages/macro-sample-addon/package.json index aee6b8cf2..d2c158049 100644 --- a/test-packages/macro-sample-addon/package.json +++ b/test-packages/macro-sample-addon/package.json @@ -36,7 +36,6 @@ "@embroider/test-support": "0.36.0", "@embroider/webpack": "0.41.0", "broccoli-asset-rev": "^3.0.0", - "ember-cli-dependency-checker": "^3.1.0", "ember-cli": "~3.26.1", "ember-cli-eslint": "^5.1.0", "ember-cli-inject-live-reload": "^1.8.2", @@ -49,7 +48,7 @@ "ember-maybe-import-regenerator": "^0.1.6", "ember-qunit": "^4.4.1", "ember-resolver": "^5.0.1", - "ember-source": "~3.10.0", + "ember-source": "~3.16.0", "ember-source-channel-url": "^1.1.0", "ember-try": "^1.0.0", "ember-welcome-page": "^4.0.0", @@ -67,4 +66,4 @@ "volta": { "extends": "../../package.json" } -} +} \ No newline at end of file diff --git a/yarn.lock b/yarn.lock index 6b609b01b..a59c1aefe 100644 --- a/yarn.lock +++ b/yarn.lock @@ -17045,6 +17045,13 @@ resolve-package-path@^3.1.0: path-root "^0.1.1" resolve "^1.17.0" +resolve-package-path@^4.0.1: + version "4.0.1" + resolved "https://registry.yarnpkg.com/resolve-package-path/-/resolve-package-path-4.0.1.tgz#0e77271e06c8cc41740d28ef974806a77fdc8880" + integrity sha512-2gb/yU2fSfX22pjDYyevzyOKK9q72XKUFqlAsrfPzZArM4JkIH/Qcme4n3EbaZttObWm/fIFLbPxrXIyiL8wdQ== + dependencies: + path-root "^0.1.1" + resolve-path@^1.4.0: version "1.4.0" resolved "https://registry.yarnpkg.com/resolve-path/-/resolve-path-1.4.0.tgz#c4bda9f5efb2fce65247873ab36bb4d834fe16f7" From e988688e62f105aab1c7edc30fbb6b196e3edd2a Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Wed, 9 Jun 2021 17:13:35 -0600 Subject: [PATCH 2/2] Fix babel plugin cache eviction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Today, babel’s caches (via babel-loader today) expires caches only if the options passed to it change, unfortunately if you upgrade a plugin without changing the configuration options and the file contents remains the same, the cache will not be evicted. This can lead to spurious caching problems if the associated plugins behavior has changed. To address this defect we now include (when available) the package.json#version of the babel plugins used in our own no-op babel plugin’s configuration. This new no-op plugin is then inserted at the end of the configuration in question. --- packages/core/src/app.ts | 40 +++++++++++++++-- .../core/src/babel-plugin-cache-busting.ts | 11 +++++ packages/core/src/portable.ts | 6 +-- packages/core/tests/app.test.ts | 45 ++++++++++++++++++- packages/core/tests/portable.test.ts | 9 ++-- test-packages/macro-sample-addon/package.json | 5 ++- 6 files changed, 102 insertions(+), 14 deletions(-) create mode 100644 packages/core/src/babel-plugin-cache-busting.ts diff --git a/packages/core/src/app.ts b/packages/core/src/app.ts index 810ac8a16..746dff0a1 100644 --- a/packages/core/src/app.ts +++ b/packages/core/src/app.ts @@ -35,7 +35,7 @@ import partition from 'lodash/partition'; import mergeWith from 'lodash/mergeWith'; import cloneDeep from 'lodash/cloneDeep'; import type { Params as InlineBabelParams } from './babel-plugin-inline-hbs-node'; -import { PortableHint } from './portable'; +import { PortableHint, maybeNodeModuleVersion } from './portable'; import escapeRegExp from 'escape-string-regexp'; import { getEmberExports } from './load-ember-template-compiler'; @@ -144,6 +144,38 @@ export function excludeDotFiles(files: string[]) { return files.filter(file => !file.startsWith('.') && !file.includes('/.')); } +export const CACHE_BUSTING_PLUGIN = { + path: require.resolve('./babel-plugin-cache-busting'), + version: readJSONSync(`${__dirname}/../package.json`).version, +}; + +export function addCachablePlugin(babelConfig: TransformOptions) { + if (Array.isArray(babelConfig.plugins) && babelConfig.plugins.length > 0) { + const plugins = Object.create(null); + plugins[CACHE_BUSTING_PLUGIN.path] = CACHE_BUSTING_PLUGIN.version; + + for (const plugin of babelConfig.plugins) { + let absolutePathToPlugin: string; + if (Array.isArray(plugin) && typeof plugin[0] === 'string') { + absolutePathToPlugin = plugin[0] as string; + } else if (typeof plugin === 'string') { + absolutePathToPlugin = plugin; + } else { + throw new Error(`[Embroider] a babel plugin without an absolute path was from: ${plugin}`); + } + + plugins[absolutePathToPlugin] = maybeNodeModuleVersion(absolutePathToPlugin); + } + + babelConfig.plugins.push([ + CACHE_BUSTING_PLUGIN.path, + { + plugins, + }, + ]); + } +} + class ParsedEmberAsset { kind: 'parsed-ember' = 'parsed-ember'; relativePath: string; @@ -383,7 +415,9 @@ export class AppBuilder { { absoluteRuntime: __dirname, useESModules: true, regenerator: false }, ]); - return makePortable(babel, { basedir: this.root }, this.portableHints); + const portable = makePortable(babel, { basedir: this.root }, this.portableHints); + addCachablePlugin(portable.config); + return portable; } private adjustImportsPlugin(engines: Engine[]): PluginItem { @@ -926,7 +960,7 @@ export class AppBuilder { return { requireFile: cursor, useMethod: hint.useMethod, - packageVersion: readJSONSync(cursor).version, + packageVersion: maybeNodeModuleVersion(cursor), }; }); } diff --git a/packages/core/src/babel-plugin-cache-busting.ts b/packages/core/src/babel-plugin-cache-busting.ts new file mode 100644 index 000000000..16d25884e --- /dev/null +++ b/packages/core/src/babel-plugin-cache-busting.ts @@ -0,0 +1,11 @@ +export default function makePlugin(): any { + // Dear future @rwjblue, + // + // This plugin exists as a sentinel plugin which has no behavior, but + // provides a position in the babel configuration to include cache busting + // meta-data about other plugins. Specifically their versions. + // + // Yours sincerely, + // Contributor + return {}; +} diff --git a/packages/core/src/portable.ts b/packages/core/src/portable.ts index d4bce561b..c2f6d9933 100644 --- a/packages/core/src/portable.ts +++ b/packages/core/src/portable.ts @@ -2,7 +2,6 @@ import mapValues from 'lodash/mapValues'; import assertNever from 'assert-never'; import { Memoize } from 'typescript-memoize'; import resolvePackagePath from 'resolve-package-path'; -import { readJSONSync } from 'fs-extra'; export const protocol = '__embroider_portable_values__'; const { globalValues, nonce } = setupGlobals(); @@ -25,10 +24,9 @@ export function maybeNodeModuleVersion(path: string) { const packagePath = findUpPackagePath(path); if (packagePath === null) { - // no package was found - return undefined; // should this bust the cache or ... ? + throw new Error(`Could not find package.json for '${path}'`); } else { - return readJSONSync(packagePath).version; + return require(packagePath).version; // eslint-disable-line @typescript-eslint/no-require-imports } } diff --git a/packages/core/tests/app.test.ts b/packages/core/tests/app.test.ts index cd698dd62..9b0704233 100644 --- a/packages/core/tests/app.test.ts +++ b/packages/core/tests/app.test.ts @@ -1,4 +1,4 @@ -import { excludeDotFiles } from '../src/app'; +import { excludeDotFiles, addCachablePlugin, CACHE_BUSTING_PLUGIN } from '../src/app'; describe('dot files can be excluded', () => { test('excludeDotFiles works', () => { @@ -10,3 +10,46 @@ describe('dot files can be excluded', () => { expect(excludeDotFiles(['foo/bar/baz/.foo.js'])).toEqual([]); }); }); + +describe('cacheable-plugin', function () { + test('noop', function () { + const input = {}; + addCachablePlugin(input); + expect(input).toEqual({}); + }); + + test('no plugins', function () { + const input = { plugins: [] }; + addCachablePlugin(input); + expect(input).toEqual({ plugins: [] }); + }); + + test('some plugins', function () { + const input = { + plugins: [__dirname, [__dirname, []], [`${__dirname}/../`, []], __dirname, [__dirname, []]], + }; + + addCachablePlugin(input); + + expect(input).toEqual({ + plugins: [ + __dirname, + [__dirname, []], + [`${__dirname}/../`, []], + __dirname, + [__dirname, []], + + [ + CACHE_BUSTING_PLUGIN.path, + { + plugins: { + [CACHE_BUSTING_PLUGIN.path]: CACHE_BUSTING_PLUGIN.version, + [__dirname]: CACHE_BUSTING_PLUGIN.version, + [`${__dirname}/../`]: CACHE_BUSTING_PLUGIN.version, + }, + }, + ], + ], + }); + }); +}); diff --git a/packages/core/tests/portable.test.ts b/packages/core/tests/portable.test.ts index 813dc555c..355e08700 100644 --- a/packages/core/tests/portable.test.ts +++ b/packages/core/tests/portable.test.ts @@ -1,14 +1,15 @@ import { maybeNodeModuleVersion } from '../src/portable'; import { readJSONSync } from 'fs-extra'; -const EMBROIDER_CORE_VERSION = readJSONSync('../../package.json').version; +const EMBROIDER_CORE_VERSION = readJSONSync(`${__dirname}/../package.json`).version; describe('maybeNodeModuleVersion', () => { test('it', () => { - expect(maybeNodeModuleVersion('/dev/null')).toEqual(undefined); - expect(maybeNodeModuleVersion('/does/not/exist')).toEqual(undefined); + expect(() => maybeNodeModuleVersion('/dev/null')).toThrow(/Could not find package.json for '\/dev\/null'/); + expect(() => maybeNodeModuleVersion('/does/not/exist')).toThrow( + /Could not find package.json for '\/does\/not\/exist'/ + ); expect(maybeNodeModuleVersion(__dirname)).toEqual(EMBROIDER_CORE_VERSION); expect(maybeNodeModuleVersion(__filename)).toEqual(EMBROIDER_CORE_VERSION); }); }); - diff --git a/test-packages/macro-sample-addon/package.json b/test-packages/macro-sample-addon/package.json index d2c158049..aee6b8cf2 100644 --- a/test-packages/macro-sample-addon/package.json +++ b/test-packages/macro-sample-addon/package.json @@ -36,6 +36,7 @@ "@embroider/test-support": "0.36.0", "@embroider/webpack": "0.41.0", "broccoli-asset-rev": "^3.0.0", + "ember-cli-dependency-checker": "^3.1.0", "ember-cli": "~3.26.1", "ember-cli-eslint": "^5.1.0", "ember-cli-inject-live-reload": "^1.8.2", @@ -48,7 +49,7 @@ "ember-maybe-import-regenerator": "^0.1.6", "ember-qunit": "^4.4.1", "ember-resolver": "^5.0.1", - "ember-source": "~3.16.0", + "ember-source": "~3.10.0", "ember-source-channel-url": "^1.1.0", "ember-try": "^1.0.0", "ember-welcome-page": "^4.0.0", @@ -66,4 +67,4 @@ "volta": { "extends": "../../package.json" } -} \ No newline at end of file +}