Skip to content

Commit

Permalink
fix(compiler): don't break HMR by mangling CSS (#3517)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
alicewriteswrongs committed Aug 15, 2022
1 parent 103ec60 commit f5b2b69
Show file tree
Hide file tree
Showing 7 changed files with 229 additions and 12 deletions.
6 changes: 6 additions & 0 deletions src/compiler/bundle/bundle-interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/bundle/bundle-output.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions src/compiler/bundle/dev-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
54 changes: 50 additions & 4 deletions src/compiler/bundle/ext-transforms-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
140 changes: 140 additions & 0 deletions src/compiler/bundle/test/ext-transforms-plugin.spec.ts
Original file line number Diff line number Diff line change
@@ -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);
}
);
});
});
});
6 changes: 3 additions & 3 deletions src/compiler/bundle/worker-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -138,7 +138,7 @@ interface WorkerMeta {
}

const getWorker = async (
config: d.Config,
config: d.ValidatedConfig,
compilerCtx: d.CompilerCtx,
buildCtx: d.BuildCtx,
ctx: PluginContext,
Expand All @@ -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,
Expand Down
23 changes: 22 additions & 1 deletion src/compiler/transformers/stencil-import-path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit f5b2b69

Please sign in to comment.