review feedback

Author: ehrencrona

src/compiler/preprocess/index.ts

@@ -3,10 +3,10 @@ import { getLocator } from 'locate-character';
 import { MappedCode, SourceLocation, sourcemap_add_offset, combine_sourcemaps } from '../utils/mapped_code';
 import { decode_map } from './decode_sourcemap';
 import { replace_in_code, Source } from './replace_in_code';
-import { Preprocessor, PreprocessorGroup, Processed } from './types';
+import { MarkupPreprocessor, Preprocessor, PreprocessorGroup, Processed } from './types';
 
 interface SourceUpdate {
-	string: string;
+	string?: string;
 	map?: DecodedSourceMap;
 	dependencies?: string[];
 }
@@ -18,7 +18,7 @@ function get_file_basename(filename: string) {
 /**
  * Represents intermediate states of the preprocessing.
  */
-class PreprocessResult {
+class PreprocessResult implements Source {
 	// sourcemap_list is sorted in reverse order from last map (index 0) to first map (index -1)
 	// so we use sourcemap_list.unshift() to add new maps
 	// https://github.com/ampproject/remapping#multiple-transformations-of-a-file
@@ -36,8 +36,10 @@ class PreprocessResult {
 	}
 
 	update_source({ string: source, map, dependencies }: SourceUpdate) {
-		this.source = source;
-		this.get_location = getLocator(source);
+		if (source) {
+			this.source = source;
+			this.get_location = getLocator(source);
+		}
 
 		if (map) {
 			this.sourcemap_list.unshift(map);
@@ -69,16 +71,12 @@ class PreprocessResult {
 /**
  * Convert preprocessor output for the tag content into MappedCode
  */
-function processed_content_to_code(
-	processed: Processed,
-	location: SourceLocation,
-	file_basename: string
-): MappedCode {
+function processed_content_to_code(processed: Processed, location: SourceLocation, file_basename: string): MappedCode {
 	// Convert the preprocessed code and its sourcemap to a MappedCode
 	let decoded_map: DecodedSourceMap;
 	if (processed.map) {
 		decoded_map = decode_map(processed);
-		
+
 		// offset only segments pointing at original component source
 		const source_index = decoded_map.sources.indexOf(file_basename);
 		if (source_index !== -1) {
@@ -97,7 +95,8 @@ function processed_tag_to_code(
 	processed: Processed,
 	tag_name: 'style' | 'script',
 	attributes: string,
-	{ source, file_basename, get_location }: Source): MappedCode {
+	{ source, file_basename, get_location }: Source
+): MappedCode {
 	const build_mapped_code = (source: string, offset: number) =>
 		MappedCode.from_source(file_basename, source, get_location(offset));
 
@@ -114,16 +113,19 @@ function processed_tag_to_code(
 
 function parse_tag_attributes(str: string) {
 	// note: won't work with attribute values containing spaces.
-	return str.split(/\s+/).filter(Boolean).reduce((attrs, attr) => {
-		const [key, value] = attr.split('=');
-		const [,unquoted] = value && value.match(/^['"](.*)['"]$/) || [];
-		
-		return {...attrs, [key]: unquoted ?? value ?? true};
-	}, {});
+	return str
+		.split(/\s+/)
+		.filter(Boolean)
+		.reduce((attrs, attr) => {
+			const [key, value] = attr.split('=');
+			const [, unquoted] = (value && value.match(/^['"](.*)['"]$/)) || [];
+
+			return { ...attrs, [key]: unquoted ?? value ?? true };
+		}, {});
 }
 
-/** 
- * Calculate the updates required to process all instances of the specified tag. 
+/**
+ * Calculate the updates required to process all instances of the specified tag.
  */
 async function process_tag(
 	tag_name: 'style' | 'script',
@@ -144,8 +146,7 @@ async function process_tag(
 		content = '',
 		tag_offset: number
 	): Promise<MappedCode> {
-		const no_change = () =>
-			MappedCode.from_source(file_basename, tag_with_content, get_location(tag_offset));
+		const no_change = () => MappedCode.from_source(file_basename, tag_with_content, get_location(tag_offset));
 
 		if (!attributes && !content) return no_change();
 
@@ -155,14 +156,43 @@ async function process_tag(
 			filename
 		});
 
-		if (processed && processed.dependencies) dependencies.push(...processed.dependencies);
-		if (!processed || !processed.map && processed.code === content) return no_change();
+		if (!processed) return no_change();
+		if (processed.dependencies) dependencies.push(...processed.dependencies);
+		if (!processed.map && processed.code === content) return no_change();
 
-		return processed_tag_to_code(processed, tag_name, attributes,
-			{source: content, get_location: offset => source.get_location(offset + tag_offset), filename, file_basename});
+		return processed_tag_to_code(processed, tag_name, attributes, {
+			source: content,
+			get_location: offset => source.get_location(offset + tag_offset),
+			filename,
+			file_basename
+		});
 	}
 
-	return {...await replace_in_code(tag_regex, process_single_tag, source), dependencies};
+	const { string, map } = await replace_in_code(tag_regex, process_single_tag, source);
+
+	return { string, map, dependencies };
+}
+
+async function process_markup(filename: string, process: MarkupPreprocessor, source: Source) {
+	const processed = await process({
+		content: source.source,
+		filename
+	});
+
+	if (processed) {
+		return {
+			string: processed.code,
+			map: processed.map
+				? // TODO: can we use decode_sourcemap?
+				  typeof processed.map === 'string'
+					? JSON.parse(processed.map)
+					: processed.map
+				: undefined,
+			dependencies: processed.dependencies
+		};
+	} else {
+		return {};
+	}
 }
 
 export default async function preprocess(
@@ -173,9 +203,7 @@ export default async function preprocess(
 	// @ts-ignore todo: doublecheck
 	const filename = (options && options.filename) || preprocessor.filename; // legacy
 
-	const preprocessors = preprocessor
-		? Array.isArray(preprocessor) ? preprocessor : [preprocessor]
-		: [];
+	const preprocessors = preprocessor ? (Array.isArray(preprocessor) ? preprocessor : [preprocessor]) : [];
 
 	const markup = preprocessors.map(p => p.markup).filter(Boolean);
 	const script = preprocessors.map(p => p.script).filter(Boolean);
@@ -187,23 +215,7 @@ export default async function preprocess(
 	// to make debugging easier = detect low-resolution sourcemaps in fn combine_mappings
 
 	for (const process of markup) {
-		const processed = await process({
-			content: result.source,
-			filename
-		});
-
-		if (!processed) continue;
-
-		result.update_source({
-			string: processed.code,
-			map: processed.map
-				? // TODO: can we use decode_sourcemap?
-				  typeof processed.map === 'string'
-					? JSON.parse(processed.map)
-					: processed.map
-				: undefined,
-			dependencies: processed.dependencies
-		});
+		result.update_source(await process_markup(filename, process, result));
 	}
 
 	for (const process of script) {

src/compiler/preprocess/types.ts

@@ -5,14 +5,19 @@ export interface Processed {
 	toString?: () => string;
 }
 
-export interface PreprocessorGroup {
-	markup?: (options: { content: string; filename: string }) => Processed | Promise<Processed>;
-	style?: Preprocessor;
-	script?: Preprocessor;
-}
+export type MarkupPreprocessor = (options: {
+	content: string;
+	filename: string;
+}) => Processed | Promise<Processed>;
 
 export type Preprocessor = (options: {
 	content: string;
 	attributes: Record<string, string | boolean>;
 	filename?: string;
 }) => Processed | Promise<Processed>;
+
+export interface PreprocessorGroup {
+	markup?: MarkupPreprocessor;
+	style?: Preprocessor;
+	script?: Preprocessor;
+}