From f5b2b69c23de044825fccb054610a52a345415e4 Mon Sep 17 00:00:00 2001 From: Alice Date: Mon, 15 Aug 2022 16:00:16 -0400 Subject: [PATCH] fix(compiler): don't break HMR by mangling CSS (#3517) This fixes an issue (#3461) where CSS is being inappropriately mangled when using the hot-reload dev server. The issue has details about exactly what the problem is, but basically in short if you are using the `dist-hydrate-script` output target and running the dev server then any change to a CSS file will cause all styling to be wiped from the related component in the browser, making for a pretty inadequate developer experience. This commit fixes the issue by changing the conditions under which original CSS selectors are commented out --- src/compiler/bundle/bundle-interface.ts | 6 + src/compiler/bundle/bundle-output.ts | 4 +- src/compiler/bundle/dev-module.ts | 8 +- src/compiler/bundle/ext-transforms-plugin.ts | 54 ++++++- .../bundle/test/ext-transforms-plugin.spec.ts | 140 ++++++++++++++++++ src/compiler/bundle/worker-plugin.ts | 6 +- .../transformers/stencil-import-path.ts | 23 ++- 7 files changed, 229 insertions(+), 12 deletions(-) create mode 100644 src/compiler/bundle/test/ext-transforms-plugin.spec.ts diff --git a/src/compiler/bundle/bundle-interface.ts b/src/compiler/bundle/bundle-interface.ts index d633254642c..db3a02f03b8 100644 --- a/src/compiler/bundle/bundle-interface.ts +++ b/src/compiler/bundle/bundle-interface.ts @@ -2,6 +2,12 @@ import type { BuildConditionals } from '../../declarations'; import type { SourceFile, TransformerFactory } from 'typescript'; import type { PreserveEntrySignaturesOption } from 'rollup'; +/** + * Options for bundled output passed on Rollup + * + * This covers the ID for the bundle, the platform it runs on, input modules, + * and more + */ export interface BundleOptions { id: string; conditionals?: BuildConditionals; diff --git a/src/compiler/bundle/bundle-output.ts b/src/compiler/bundle/bundle-output.ts index f259596eba9..701e8b6ac91 100644 --- a/src/compiler/bundle/bundle-output.ts +++ b/src/compiler/bundle/bundle-output.ts @@ -19,7 +19,7 @@ import { userIndexPlugin } from './user-index-plugin'; import { workerPlugin } from './worker-plugin'; export const bundleOutput = async ( - config: d.Config, + config: d.ValidatedConfig, compilerCtx: d.CompilerCtx, buildCtx: d.BuildCtx, bundleOpts: BundleOptions @@ -49,7 +49,7 @@ export const bundleOutput = async ( * @returns the rollup options to be used */ export const getRollupOptions = ( - config: d.Config, + config: d.ValidatedConfig, compilerCtx: d.CompilerCtx, buildCtx: d.BuildCtx, bundleOpts: BundleOptions diff --git a/src/compiler/bundle/dev-module.ts b/src/compiler/bundle/dev-module.ts index 2b101edbe9e..5d94b174618 100644 --- a/src/compiler/bundle/dev-module.ts +++ b/src/compiler/bundle/dev-module.ts @@ -56,7 +56,11 @@ const getPackageJsonPath = (resolvedPath: string, importee: string): string => { return null; }; -export const compilerRequest = async (config: d.Config, compilerCtx: d.CompilerCtx, data: d.CompilerRequest) => { +export const compilerRequest = async ( + config: d.ValidatedConfig, + compilerCtx: d.CompilerCtx, + data: d.CompilerRequest +) => { const results: d.CompilerRequestResponse = { path: data.path, nodeModuleId: null, @@ -126,7 +130,7 @@ export const compilerRequest = async (config: d.Config, compilerCtx: d.CompilerC }; const bundleDevModule = async ( - config: d.Config, + config: d.ValidatedConfig, compilerCtx: d.CompilerCtx, parsedUrl: ParsedDevModuleUrl, results: d.CompilerRequestResponse diff --git a/src/compiler/bundle/ext-transforms-plugin.ts b/src/compiler/bundle/ext-transforms-plugin.ts index c345f0aa4f7..08e955ef57c 100644 --- a/src/compiler/bundle/ext-transforms-plugin.ts +++ b/src/compiler/bundle/ext-transforms-plugin.ts @@ -7,8 +7,18 @@ import { parseImportPath } from '../transformers/stencil-import-path'; import type { Plugin } from 'rollup'; import { runPluginTransformsEsmImports } from '../plugin/plugin'; +/** + * A Rollup plugin which bundles up some transformation of CSS imports as well + * as writing some files to disk for the `DIST_COLLECTION` output target. + * + * @param config a user-supplied configuration + * @param compilerCtx the current compiler context + * @param buildCtx the current build context + * @param bundleOpts bundle options for Rollup + * @returns a Rollup plugin which carries out the necessary work + */ export const extTransformsPlugin = ( - config: d.Config, + config: d.ValidatedConfig, compilerCtx: d.CompilerCtx, buildCtx: d.BuildCtx, bundleOpts: BundleOptions @@ -16,14 +26,31 @@ export const extTransformsPlugin = ( return { name: 'extTransformsPlugin', + /** + * A custom function targeting the `transform` build hook in Rollup. See here for details: + * https://rollupjs.org/guide/en/#transform + * + * Here we are ignoring the first argument (which contains the module's source code) and + * only looking at the `id` argument. We use that `id` to get information about the module + * in question from disk ourselves so that we can then do some transformations on it. + * + * @param _ an unused parameter (normally the code for a given module) + * @param id the id of a module + * @returns metadata for Rollup or null if no transformation should be done + */ async transform(_, id) { if (/\0/.test(id)) { return null; } + // The `id` here was possibly previously updated using + // `serializeImportPath` to annotate the filepath with various metadata + // serialized to query-params. If that was done for this particular `id` + // then the `data` prop will not be null. const { data } = parseImportPath(id); + if (data != null) { - let cmp: d.ComponentCompilerMeta; + let cmp: d.ComponentCompilerMeta | undefined = undefined; const filePath = normalizeFsPath(id); const code = await compilerCtx.fs.readFile(filePath); if (typeof code !== 'string') { @@ -31,7 +58,24 @@ export const extTransformsPlugin = ( } const pluginTransforms = await runPluginTransformsEsmImports(config, compilerCtx, buildCtx, code, filePath); - const commentOriginalSelector = bundleOpts.platform === 'hydrate' && data.encapsulation === 'shadow'; + + // We need to check whether the current build is a dev-mode watch build w/ HMR enabled in + // order to know how we'll want to set `commentOriginalSelector` (below). If we are doing + // a hydrate build we need to set this to `true` because commenting-out selectors is what + // gives us support for scoped CSS w/ hydrated components (we don't support shadow DOM and + // styling via that route for them). However, we don't want to comment selectors in dev + // mode when using HMR in the browser, since there we _do_ support putting stylesheets into + // the shadow DOM and commenting out e.g. the `:host` selector in those stylesheets will + // break components' CSS when an HMR update is sent to the browser. + // + // See https://github.com/ionic-team/stencil/issues/3461 for details + const isDevWatchHMRBuild = + config.flags.watch && + config.flags.dev && + config.flags.serve && + (config.devServer?.reloadStrategy ?? null) === 'hmr'; + const commentOriginalSelector = + bundleOpts.platform === 'hydrate' && data.encapsulation === 'shadow' && !isDevWatchHMRBuild; if (data.tag) { cmp = buildCtx.components.find((c) => c.tagName === data.tag); @@ -39,9 +83,11 @@ export const extTransformsPlugin = ( if (moduleFile) { const collectionDirs = config.outputTargets.filter(isOutputTargetDistCollection); - const relPath = relative(config.srcDir, pluginTransforms.id); + // If we found a `moduleFile` in the module map above then we + // should write the transformed CSS file (found in the return value + // of `runPluginTransformsEsmImports`, above) to disk. await Promise.all( collectionDirs.map(async (outputTarget) => { const collectionPath = join(outputTarget.collectionDir, relPath); diff --git a/src/compiler/bundle/test/ext-transforms-plugin.spec.ts b/src/compiler/bundle/test/ext-transforms-plugin.spec.ts new file mode 100644 index 00000000000..913102d4d1b --- /dev/null +++ b/src/compiler/bundle/test/ext-transforms-plugin.spec.ts @@ -0,0 +1,140 @@ +import { mockBuildCtx, mockCompilerCtx, mockModule, mockValidatedConfig } from '@stencil/core/testing'; +import { stubComponentCompilerMeta } from '../../types/tests/ComponentCompilerMeta.stub'; +import { BundleOptions } from '../bundle-interface'; +import { extTransformsPlugin } from '../ext-transforms-plugin'; +import * as importPathLib from '../../transformers/stencil-import-path'; +import { normalizePath } from '@utils'; + +describe('extTransformsPlugin', () => { + function setup(bundleOptsOverrides: Partial = {}) { + const config = mockValidatedConfig({ + plugins: [], + outputTargets: [ + { + type: 'dist-collection', + dir: 'dist/', + collectionDir: 'dist/collectionDir', + }, + ], + srcDir: '/some/stubbed/path', + }); + const compilerCtx = mockCompilerCtx(config); + const buildCtx = mockBuildCtx(config, compilerCtx); + + const compilerComponentMeta = stubComponentCompilerMeta({ + tagName: 'my-component', + componentClassName: 'MyComponent', + }); + + buildCtx.components = [compilerComponentMeta]; + + compilerCtx.moduleMap.set( + compilerComponentMeta.sourceFilePath, + mockModule({ + cmps: [compilerComponentMeta], + }) + ); + + const bundleOpts: BundleOptions = { + id: 'test-bundle', + platform: 'client', + inputs: {}, + ...bundleOptsOverrides, + }; + + const cssText = ':host { text: pink; }'; + + // mock out the read for our CSS + jest.spyOn(compilerCtx.fs, 'readFile').mockResolvedValue(cssText); + + // mock out compilerCtx.worker.transformCssToEsm because 1) we want to + // test what arguments are passed to it and 2) calling it un-mocked causes + // the infamous autoprefixer-spew-issue :( + const transformCssToEsmSpy = jest.spyOn(compilerCtx.worker, 'transformCssToEsm').mockResolvedValue({ + styleText: cssText, + output: cssText, + map: null, + diagnostics: [], + imports: [], + defaultVarName: 'foo', + styleDocs: [], + }); + + const writeFileSpy = jest.spyOn(compilerCtx.fs, 'writeFile'); + return { + plugin: extTransformsPlugin(config, compilerCtx, buildCtx, bundleOpts), + config, + compilerCtx, + buildCtx, + bundleOpts, + writeFileSpy, + transformCssToEsmSpy, + cssText, + }; + } + + describe('transform function', () => { + it('should set name', () => { + expect(setup().plugin.name).toBe('extTransformsPlugin'); + }); + + it('should return early if no data can be gleaned from the id', async () => { + const { plugin } = setup(); + // @ts-ignore we're testing something which shouldn't normally happen, + // but might if an argument of the wrong type were passed as `id` + const parseSpy = jest.spyOn(importPathLib, 'parseImportPath').mockReturnValue({ data: null }); + // @ts-ignore the Rollup plugins expect to be called in a Rollup context + expect(await plugin.transform('asdf', 'foo.css')).toBe(null); + parseSpy.mockRestore(); + }); + + it('should write CSS files if associated with a tag', async () => { + const { plugin, writeFileSpy } = setup(); + + // @ts-ignore the Rollup plugins expect to be called in a Rollup context + await plugin.transform('asdf', '/some/stubbed/path/foo.css?tag=my-component'); + + const [path, css] = writeFileSpy.mock.calls[0]; + + expect(normalizePath(path)).toBe('./dist/collectionDir/foo.css'); + + expect(css).toBe(':host { text: pink; }'); + }); + + describe('passing `commentOriginalSelector` to `transformCssToEsm`', () => { + it.each([ + [false, 'tag=my-component&encapsulation=scoped'], + [true, 'tag=my-component&encapsulation=shadow'], + [false, 'tag=my-component'], + ])('should pass true if %p and hydrate', async (expectation, queryParams) => { + const { plugin, transformCssToEsmSpy } = setup({ platform: 'hydrate' }); + // @ts-ignore the Rollup plugins expect to be called in a Rollup context + await plugin.transform('asdf', `/some/stubbed/path/foo.css?${queryParams}`); + expect(transformCssToEsmSpy.mock.calls[0][0].commentOriginalSelector).toBe(expectation); + }); + + it('should pass false if shadow, hydrate, but using HMR in dev watch mode', async () => { + const { plugin, transformCssToEsmSpy, config } = setup({ platform: 'hydrate' }); + + config.flags.watch = true; + config.flags.dev = true; + config.flags.serve = true; + config.devServer = { reloadStrategy: 'hmr' }; + + // @ts-ignore the Rollup plugins expect to be called in a Rollup context + await plugin.transform('asdf', '/some/stubbed/path/foo.css?tag=my-component&encapsulation=shadow'); + expect(transformCssToEsmSpy.mock.calls[0][0].commentOriginalSelector).toBe(false); + }); + + it.each(['tag=my-component&encapsulation=scoped', 'tag=my-component&encapsulation=shadow', 'tag=my-component'])( + 'should pass false if %p without hydrate', + async (queryParams) => { + const { plugin, transformCssToEsmSpy } = setup(); + // @ts-ignore the Rollup plugins expect to be called in a Rollup context + await plugin.transform('asdf', `/some/stubbed/path/foo.css?${queryParams}`); + expect(transformCssToEsmSpy.mock.calls[0][0].commentOriginalSelector).toBe(false); + } + ); + }); + }); +}); diff --git a/src/compiler/bundle/worker-plugin.ts b/src/compiler/bundle/worker-plugin.ts index 18567b81652..453db2e60be 100644 --- a/src/compiler/bundle/worker-plugin.ts +++ b/src/compiler/bundle/worker-plugin.ts @@ -6,7 +6,7 @@ import { optimizeModule } from '../optimize/optimize-module'; import { STENCIL_INTERNAL_ID } from './entry-alias-ids'; export const workerPlugin = ( - config: d.Config, + config: d.ValidatedConfig, compilerCtx: d.CompilerCtx, buildCtx: d.BuildCtx, platform: string, @@ -138,7 +138,7 @@ interface WorkerMeta { } const getWorker = async ( - config: d.Config, + config: d.ValidatedConfig, compilerCtx: d.CompilerCtx, buildCtx: d.BuildCtx, ctx: PluginContext, @@ -160,7 +160,7 @@ const getWorkerName = (id: string) => { }; const buildWorker = async ( - config: d.Config, + config: d.ValidatedConfig, compilerCtx: d.CompilerCtx, buildCtx: d.BuildCtx, ctx: PluginContext, diff --git a/src/compiler/transformers/stencil-import-path.ts b/src/compiler/transformers/stencil-import-path.ts index f039ba68f40..be37e6c88b6 100644 --- a/src/compiler/transformers/stencil-import-path.ts +++ b/src/compiler/transformers/stencil-import-path.ts @@ -2,7 +2,21 @@ import type { ImportData, ParsedImport, SerializeImportData } from '../../declar import { basename, dirname, isAbsolute, relative } from 'path'; import { DEFAULT_STYLE_MODE, isString, normalizePath } from '@utils'; -export const serializeImportPath = (data: SerializeImportData, styleImportData: string) => { +/** + * Serialize data about a style import to an annotated path, where + * the filename has a URL queryparams style string appended to it. + * This could look like: + * + * ``` + * './some-file.CSS?tag=my-tag&mode=ios&encapsulation=scoped'); + * ``` + * + * @param data import data to be serialized + * @param styleImportData an argument which controls whether the import data + * will be added to the path (formatted as queryparams) + * @returns a formatted string + */ +export const serializeImportPath = (data: SerializeImportData, styleImportData: string | undefined | null): string => { let p = data.importeePath; if (isString(p)) { @@ -36,6 +50,13 @@ export const serializeImportPath = (data: SerializeImportData, styleImportData: return p; }; +/** + * Parse import paths (filepaths possibly annotated w/ component metadata, + * formatted as URL queryparams) into a structured format. + * + * @param importPath an annotated import path to examine + * @returns formatted information about the import + */ export const parseImportPath = (importPath: string) => { const parsedPath: ParsedImport = { importPath,