Merge pull request #2102 from sveltejs/warnings-and-vars

move warnings and vars out of stats

src/Stats.ts

@@ -1,6 +1,3 @@
-import { Warning } from './interfaces';
-import Component from './compile/Component';
-
 const now = (typeof process !== 'undefined' && process.hrtime)
 	? () => {
 		const t = process.hrtime();
@@ -31,14 +28,11 @@ export default class Stats {
 	currentChildren: Timing[];
 	timings: Timing[];
 	stack: Timing[];
-	warnings: Warning[];
 
 	constructor() {
 		this.startTime = now();
 		this.stack = [];
 		this.currentChildren = this.timings = [];
-
-		this.warnings = [];
 	}
 
 	start(label) {
@@ -67,46 +61,13 @@ export default class Stats {
 		this.currentChildren = this.currentTiming ? this.currentTiming.children : this.timings;
 	}
 
-	render(component: Component) {
+	render() {
 		const timings = Object.assign({
 			total: now() - this.startTime
 		}, collapseTimings(this.timings));
 
-		// TODO would be good to have this info even
-		// if options.generate is false
-		const imports = component && component.imports.map(node => {
-			return {
-				source: node.source.value,
-				specifiers: node.specifiers.map(specifier => {
-					return {
-						name: (
-							specifier.type === 'ImportDefaultSpecifier' ? 'default' :
-							specifier.type === 'ImportNamespaceSpecifier' ? '*' :
-							specifier.imported.name
-						),
-						as: specifier.local.name
-					};
-				})
-			}
-		});
-
 		return {
-			timings,
-			warnings: this.warnings,
-			vars: component.vars.filter(variable => !variable.global && !variable.implicit && !variable.internal).map(variable => ({
-				name: variable.name,
-				export_name: variable.export_name || null,
-				injected: variable.injected || false,
-				module: variable.module || false,
-				mutated: variable.mutated || false,
-				reassigned: variable.reassigned || false,
-				referenced: variable.referenced || false,
-				writable: variable.writable || false
-			}))
+			timings
 		};
 	}
-
-	warn(warning) {
-		this.warnings.push(warning);
-	}
 }

src/compile/Component.ts

@@ -11,7 +11,7 @@ import Stylesheet from './css/Stylesheet';
 import { test } from '../config';
 import Fragment from './nodes/Fragment';
 import internal_exports from './internal-exports';
-import { Node, Ast, CompileOptions, Var } from '../interfaces';
+import { Node, Ast, CompileOptions, Var, Warning } from '../interfaces';
 import error from '../utils/error';
 import getCodeFrame from '../utils/getCodeFrame';
 import flattenReference from '../utils/flattenReference';
@@ -40,6 +40,7 @@ childKeys.ExportNamedDeclaration = ['declaration', 'specifiers'];
 
 export default class Component {
 	stats: Stats;
+	warnings: Warning[];
 
 	ast: Ast;
 	source: string;
@@ -93,11 +94,13 @@ export default class Component {
 		source: string,
 		name: string,
 		compileOptions: CompileOptions,
-		stats: Stats
+		stats: Stats,
+		warnings: Warning[]
 	) {
 		this.name = name;
 
 		this.stats = stats;
+		this.warnings = warnings;
 		this.ast = ast;
 		this.source = source;
 		this.compileOptions = compileOptions;
@@ -161,7 +164,7 @@ export default class Component {
 
 		if (!compileOptions.customElement) this.stylesheet.reify();
 
-		this.stylesheet.warnOnUnusedSelectors(stats);
+		this.stylesheet.warnOnUnusedSelectors(this);
 	}
 
 	add_var(variable: Var) {
@@ -214,105 +217,121 @@ export default class Component {
 	}
 
 	generate(result: string) {
-		const { compileOptions, name } = this;
-		const { format = 'esm' } = compileOptions;
+		let js = null;
+		let css = null;
 
-		const banner = `/* ${this.file ? `${this.file} ` : ``}generated by Svelte v${"__VERSION__"} */`;
+		if (result) {
+			const { compileOptions, name } = this;
+			const { format = 'esm' } = compileOptions;
 
-		// TODO use same regex for both
-		result = result.replace(compileOptions.generate === 'ssr' ? /(@+|#+)(\w*(?:-\w*)?)/g : /(@+)(\w*(?:-\w*)?)/g, (match: string, sigil: string, name: string) => {
-			if (sigil === '@') {
-				if (internal_exports.has(name)) {
-					if (compileOptions.dev && internal_exports.has(`${name}Dev`)) name = `${name}Dev`;
-					this.helpers.add(name);
-				}
+			const banner = `/* ${this.file ? `${this.file} ` : ``}generated by Svelte v${"__VERSION__"} */`;
 
-				return this.alias(name);
-			}
+			// TODO use same regex for both
+			result = result.replace(compileOptions.generate === 'ssr' ? /(@+|#+)(\w*(?:-\w*)?)/g : /(@+)(\w*(?:-\w*)?)/g, (match: string, sigil: string, name: string) => {
+				if (sigil === '@') {
+					if (internal_exports.has(name)) {
+						if (compileOptions.dev && internal_exports.has(`${name}Dev`)) name = `${name}Dev`;
+						this.helpers.add(name);
+					}
 
-			return sigil.slice(1) + name;
-		});
+					return this.alias(name);
+				}
 
-		const importedHelpers = Array.from(this.helpers)
-			.sort()
-			.map(name => {
-				const alias = this.alias(name);
-				return { name, alias };
+				return sigil.slice(1) + name;
 			});
 
-		const module = wrapModule(
-			result,
-			format,
-			name,
-			compileOptions,
-			banner,
-			compileOptions.sveltePath,
-			importedHelpers,
-			this.imports,
-			this.vars.filter(variable => variable.module && variable.export_name).map(variable => ({
-				name: variable.name,
-				as: variable.export_name
-			})),
-			this.source
-		);
+			const importedHelpers = Array.from(this.helpers)
+				.sort()
+				.map(name => {
+					const alias = this.alias(name);
+					return { name, alias };
+				});
+
+			const module = wrapModule(
+				result,
+				format,
+				name,
+				compileOptions,
+				banner,
+				compileOptions.sveltePath,
+				importedHelpers,
+				this.imports,
+				this.vars.filter(variable => variable.module && variable.export_name).map(variable => ({
+					name: variable.name,
+					as: variable.export_name
+				})),
+				this.source
+			);
 
-		const parts = module.split('✂]');
-		const finalChunk = parts.pop();
+			const parts = module.split('✂]');
+			const finalChunk = parts.pop();
 
-		const compiled = new Bundle({ separator: '' });
+			const compiled = new Bundle({ separator: '' });
 
-		function addString(str: string) {
-			compiled.addSource({
-				content: new MagicString(str),
-			});
-		}
+			function addString(str: string) {
+				compiled.addSource({
+					content: new MagicString(str),
+				});
+			}
 
-		const { filename } = compileOptions;
+			const { filename } = compileOptions;
 
-		// special case — the source file doesn't actually get used anywhere. we need
-		// to add an empty file to populate map.sources and map.sourcesContent
-		if (!parts.length) {
-			compiled.addSource({
-				filename,
-				content: new MagicString(this.source).remove(0, this.source.length),
-			});
-		}
+			// special case — the source file doesn't actually get used anywhere. we need
+			// to add an empty file to populate map.sources and map.sourcesContent
+			if (!parts.length) {
+				compiled.addSource({
+					filename,
+					content: new MagicString(this.source).remove(0, this.source.length),
+				});
+			}
 
-		const pattern = /\[✂(\d+)-(\d+)$/;
+			const pattern = /\[✂(\d+)-(\d+)$/;
 
-		parts.forEach((str: string) => {
-			const chunk = str.replace(pattern, '');
-			if (chunk) addString(chunk);
+			parts.forEach((str: string) => {
+				const chunk = str.replace(pattern, '');
+				if (chunk) addString(chunk);
 
-			const match = pattern.exec(str);
+				const match = pattern.exec(str);
 
-			const snippet = this.code.snip(+match[1], +match[2]);
+				const snippet = this.code.snip(+match[1], +match[2]);
 
-			compiled.addSource({
-				filename,
-				content: snippet,
+				compiled.addSource({
+					filename,
+					content: snippet,
+				});
 			});
-		});
 
-		addString(finalChunk);
+			addString(finalChunk);
 
-		const css = compileOptions.customElement ?
-			{ code: null, map: null } :
-			this.stylesheet.render(compileOptions.cssOutputFilename, true);
+			css = compileOptions.customElement ?
+				{ code: null, map: null } :
+				this.stylesheet.render(compileOptions.cssOutputFilename, true);
 
-		const js = {
-			code: compiled.toString(),
-			map: compiled.generateMap({
-				includeContent: true,
-				file: compileOptions.outputFilename,
-			})
-		};
+			js = {
+				code: compiled.toString(),
+				map: compiled.generateMap({
+					includeContent: true,
+					file: compileOptions.outputFilename,
+				})
+			};
+		}
 
 		return {
-			ast: this.ast,
 			js,
 			css,
-			stats: this.stats.render(this)
+			ast: this.ast,
+			warnings: this.warnings,
+			vars: this.vars.filter(v => !v.global && !v.implicit && !v.internal).map(v => ({
+				name: v.name,
+				export_name: v.export_name || null,
+				injected: v.injected || false,
+				module: v.module || false,
+				mutated: v.mutated || false,
+				reassigned: v.reassigned || false,
+				referenced: v.referenced || false,
+				writable: v.writable || false
+			})),
+			stats: this.stats.render()
 		};
 	}
 
@@ -393,7 +412,7 @@ export default class Component {
 
 		const frame = getCodeFrame(this.source, start.line - 1, start.column);
 
-		this.stats.warn({
+		this.warnings.push({
 			code: warning.code,
 			message: warning.message,
 			frame,

src/compile/css/Stylesheet.ts

@@ -1,14 +1,11 @@
 import MagicString from 'magic-string';
 import { walk } from 'estree-walker';
-import { getLocator } from 'locate-character';
 import Selector from './Selector';
-import getCodeFrame from '../../utils/getCodeFrame';
 import hash from '../../utils/hash';
 import removeCSSPrefix from '../../utils/removeCSSPrefix';
 import Element from '../nodes/Element';
-import { Node, Ast, Warning } from '../../interfaces';
+import { Node, Ast } from '../../interfaces';
 import Component from '../Component';
-import Stats from '../../Stats';
 
 const isKeyframesNode = (node: Node) => removeCSSPrefix(node.name) === 'keyframes'
 
@@ -392,33 +389,14 @@ export default class Stylesheet {
 		});
 	}
 
-	warnOnUnusedSelectors(stats: Stats) {
-		let locator;
-
-		const handler = (selector: Selector) => {
-			const pos = selector.node.start;
-
-			if (!locator) locator = getLocator(this.source, { offsetLine: 1 });
-			const start = locator(pos);
-			const end = locator(selector.node.end);
-
-			const frame = getCodeFrame(this.source, start.line - 1, start.column);
-			const message = `Unused CSS selector`;
-
-			stats.warn({
-				code: `css-unused-selector`,
-				message,
-				frame,
-				start,
-				end,
-				pos,
-				filename: this.filename,
-				toString: () => `${message} (${start.line}:${start.column})\n${frame}`,
-			});
-		};
-
+	warnOnUnusedSelectors(component: Component) {
 		this.children.forEach(child => {
-			child.warnOnUnusedSelector(handler);
+			child.warnOnUnusedSelector((selector: Selector) => {
+				component.warn(selector.node, {
+					code: `css-unused-selector`,
+					message: `Unused CSS selector`
+				});
+			});
 		});
 	}
 }

src/compile/index.ts

@@ -3,7 +3,7 @@ import Stats from '../Stats';
 import parse from '../parse/index';
 import renderDOM from './render-dom/index';
 import renderSSR from './render-ssr/index';
-import { CompileOptions, Ast } from '../interfaces';
+import { CompileOptions, Ast, Warning } from '../interfaces';
 import Component from './Component';
 import fuzzymatch from '../utils/fuzzymatch';
 
@@ -24,7 +24,7 @@ const valid_options = [
 	'preserveComments'
 ];
 
-function validate_options(options: CompileOptions, stats: Stats) {
+function validate_options(options: CompileOptions, warnings: Warning[]) {
 	const { name, filename } = options;
 
 	Object.keys(options).forEach(key => {
@@ -43,7 +43,7 @@ function validate_options(options: CompileOptions, stats: Stats) {
 
 	if (name && /^[a-z]/.test(name)) {
 		const message = `options.name should be capitalised`;
-		stats.warn({
+		warnings.push({
 			code: `options-lowercase-name`,
 			message,
 			filename,
@@ -74,10 +74,11 @@ export default function compile(source: string, options: CompileOptions = {}) {
 	options = assign({ generate: 'dom', dev: false }, options);
 
 	const stats = new Stats();
+	const warnings = [];
 
 	let ast: Ast;
 
-	validate_options(options, stats);
+	validate_options(options, warnings);
 
 	stats.start('parse');
 	ast = parse(source, options);
@@ -89,17 +90,16 @@ export default function compile(source: string, options: CompileOptions = {}) {
 		source,
 		options.name || get_name(options.filename) || 'SvelteComponent',
 		options,
-		stats
+		stats,
+		warnings
 	);
 	stats.stop('create component');
 
-	if (options.generate === false) {
-		return { ast, stats: stats.render(component), js: null, css: null };
-	}
-
-	const js = options.generate === 'ssr'
-		? renderSSR(component, options)
-		: renderDOM(component, options);
+	const js = options.generate === false
+		? null
+		: options.generate === 'ssr'
+			? renderSSR(component, options)
+			: renderDOM(component, options);
 
 	return component.generate(js);
 }

test/css/index.js

@@ -68,8 +68,8 @@ describe('css', () => {
 
 			assert.equal(dom.css.code, ssr.css.code);
 
-			const dom_warnings = dom.stats.warnings.map(normalize_warning);
-			const ssr_warnings = ssr.stats.warnings.map(normalize_warning);
+			const dom_warnings = dom.warnings.map(normalize_warning);
+			const ssr_warnings = ssr.warnings.map(normalize_warning);
 
 			assert.deepEqual(dom_warnings, ssr_warnings);
 			assert.deepEqual(dom_warnings.map(normalize_warning), expected_warnings);

test/stats/index.js

@@ -19,8 +19,6 @@ describe('stats', () => {
 			const filename = `test/stats/samples/${dir}/input.svelte`;
 			const input = fs.readFileSync(filename, 'utf-8').replace(/\s+$/, '');
 
-			const expectedWarnings =
-				tryToLoadJson(`test/stats/samples/${dir}/warnings.json`) || [];
 			const expectedError = tryToLoadJson(
 				`test/stats/samples/${dir}/error.json`
 			);
@@ -31,10 +29,6 @@ describe('stats', () => {
 			try {
 				result = svelte.compile(input, config.options);
 				config.test(assert, result.stats);
-
-				if (result.stats.warnings.length || expectedWarnings.length) {
-					// TODO check warnings are added to stats.warnings
-				}
 			} catch (e) {
 				error = e;
 			}

test/stats/samples/duplicate-globals/_config.js

@@ -1,5 +0,0 @@
-export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, []);
-	},
-};

test/stats/samples/implicit-action/_config.js

@@ -1,5 +0,0 @@
-export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, []);
-	},
-};

test/stats/samples/implicit/_config.js

@@ -1,5 +0,0 @@
-export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, []);
-	},
-};

test/stats/samples/template-references/_config.js

@@ -1,5 +0,0 @@
-export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, []);
-	},
-};

test/stats/samples/undeclared/_config.js

@@ -1,5 +0,0 @@
-export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, []);
-	},
-};

test/validator/index.js

@@ -24,21 +24,19 @@ describe("validate", () => {
 			let error;
 
 			try {
-				const { stats } = svelte.compile(input, {
+				let { warnings } = svelte.compile(input, {
 					dev: config.dev,
 					legacy: config.legacy,
 					generate: false
 				});
 
-				const warnings = stats.warnings.map(w => ({
+				assert.deepEqual(warnings.map(w => ({
 					code: w.code,
 					message: w.message,
 					pos: w.pos,
 					start: w.start,
 					end: w.end
-				}));
-
-				assert.deepEqual(warnings, expected_warnings);
+				})), expected_warnings);
 			} catch (e) {
 				error = e;
 			}
@@ -78,12 +76,12 @@ describe("validate", () => {
 	});
 
 	it("warns if options.name is not capitalised", () => {
-		const { stats } = svelte.compile("<div></div>", {
+		const { warnings } = svelte.compile("<div></div>", {
 			name: "lowercase",
 			generate: false
 		});
 
-		assert.deepEqual(stats.warnings.map(w => ({
+		assert.deepEqual(warnings.map(w => ({
 			code: w.code,
 			message: w.message
 		})), [{
@@ -93,11 +91,11 @@ describe("validate", () => {
 	});
 
 	it("does not warn if options.name begins with non-alphabetic character", () => {
-		const { stats } = svelte.compile("<div></div>", {
+		const { warnings } = svelte.compile("<div></div>", {
 			name: "_",
 			generate: false
 		});
 
-		assert.deepEqual(stats.warnings, []);
+		assert.deepEqual(warnings, []);
 	});
 });

test/vars/index.js

@@ -0,0 +1,60 @@
+import * as fs from 'fs';
+import * as assert from 'assert';
+import { svelte, loadConfig, tryToLoadJson } from '../helpers.js';
+
+describe('vars', () => {
+	fs.readdirSync('test/vars/samples').forEach(dir => {
+		if (dir[0] === '.') return;
+
+		// add .solo to a sample directory name to only run that test
+		const solo = /\.solo/.test(dir);
+		const skip = /\.skip/.test(dir);
+
+		if (solo && process.env.CI) {
+			throw new Error('Forgot to remove `solo: true` from test');
+		}
+
+		(solo ? it.only : skip ? it.skip : it)(dir, () => {
+			const config = loadConfig(`./vars/samples/${dir}/_config.js`);
+			const filename = `test/vars/samples/${dir}/input.svelte`;
+			const input = fs.readFileSync(filename, 'utf-8').replace(/\s+$/, '');
+
+			const expectedError = tryToLoadJson(
+				`test/vars/samples/${dir}/error.json`
+			);
+
+			let result;
+			let error;
+
+			try {
+				result = svelte.compile(input, config.options);
+				config.test(assert, result.vars);
+			} catch (e) {
+				error = e;
+			}
+
+			if (error || expectedError) {
+				if (error && !expectedError) {
+					throw error;
+				}
+
+				if (expectedError && !error) {
+					throw new Error(`Expected an error: ${expectedError.message}`);
+				}
+
+				assert.equal(error.message, expectedError.message);
+				assert.deepEqual(error.start, expectedError.start);
+				assert.deepEqual(error.end, expectedError.end);
+				assert.equal(error.pos, expectedError.pos);
+			}
+		});
+	});
+
+	it('returns a vars object when options.generate is false', () => {
+		const { vars } = svelte.compile('', {
+			generate: false
+		});
+
+		assert.ok(Array.isArray(vars));
+	});
+});

test/vars/samples/duplicate-globals/_config.js

@@ -0,0 +1,5 @@
+export default {
+	test(assert, vars) {
+		assert.deepEqual(vars, []);
+	},
+};

test/vars/samples/duplicate-non-hoistable/_config.js

@@ -1,6 +1,6 @@
 export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, [
+	test(assert, vars) {
+		assert.deepEqual(vars, [
 			{
 				name: 'console',
 				injected: false,

test/vars/samples/duplicate-vars/_config.js

@@ -1,6 +1,6 @@
 export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, [
+	test(assert, vars) {
+		assert.deepEqual(vars, [
 			{
 				name: 'foo',
 				injected: false,

test/vars/samples/implicit-action/_config.js

@@ -0,0 +1,5 @@
+export default {
+	test(assert, vars) {
+		assert.deepEqual(vars, []);
+	},
+};

test/vars/samples/implicit-reactive/_config.js

@@ -1,6 +1,6 @@
 export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, [
+	test(assert, vars) {
+		assert.deepEqual(vars, [
 			{
 				name: 'a',
 				injected: false,

test/vars/samples/implicit/_config.js

@@ -0,0 +1,5 @@
+export default {
+	test(assert, vars) {
+		assert.deepEqual(vars, []);
+	},
+};

test/vars/samples/imports/_config.js

@@ -1,6 +1,6 @@
 export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, [
+	test(assert, vars) {
+		assert.deepEqual(vars, [
 			{
 				name: 'x',
 				export_name: null,

test/vars/samples/mutated-vs-reassigned-bindings/_config.js

@@ -1,6 +1,6 @@
 export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, [
+	test(assert, vars) {
+		assert.deepEqual(vars, [
 			{
 				name: 'count',
 				export_name: null,

test/vars/samples/mutated-vs-reassigned/_config.js

@@ -1,6 +1,6 @@
 export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, [
+	test(assert, vars) {
+		assert.deepEqual(vars, [
 			{
 				name: 'count',
 				export_name: null,

test/vars/samples/props/_config.js

@@ -1,6 +1,6 @@
 export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, [
+	test(assert, vars) {
+		assert.deepEqual(vars, [
 			{
 				name: 'name',
 				export_name: 'name',

test/vars/samples/store-referenced/_config.js

@@ -1,6 +1,6 @@
 export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, [
+	test(assert, vars) {
+		assert.deepEqual(vars, [
 			{
 				name: 'foo',
 				export_name: null,

test/vars/samples/store-unreferenced/_config.js

@@ -1,6 +1,6 @@
 export default {
-	test(assert, stats) {
-		assert.deepEqual(stats.vars, [
+	test(assert, vars) {
+		assert.deepEqual(vars, [
 			{
 				name: 'foo',
 				export_name: null,

test/vars/samples/template-references/_config.js

@@ -0,0 +1,5 @@
+export default {
+	test(assert, vars) {
+		assert.deepEqual(vars, []);
+	},
+};

test/vars/samples/undeclared/_config.js

@@ -0,0 +1,5 @@
+export default {
+	test(assert, vars) {
+		assert.deepEqual(vars, []);
+	},
+};