fix(compiler): don't break HMR by mangling CSS

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;

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

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

src/compiler/bundle/ext-transforms-plugin.ts

@@ -7,41 +7,87 @@ 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
 ): Plugin => {
   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') {
           return null;
         }
 
         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);
           const moduleFile = cmp && compilerCtx.moduleMap.get(cmp.sourceFilePath);
 
           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);

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<BundleOptions> = {}) {
+    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);
+        }
+      );
+    });
+  });
+});

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,

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,