Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid relying on buggy import order in the legacy importer #245

Merged
merged 3 commits into from
Sep 8, 2023
Merged

Avoid relying on buggy import order in the legacy importer #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac8bff7
Avoid relying on buggy import order in the legacy importer
nex3 Sep 7, 2023
210835b
Don't use a legacy URL prefix when there's no legacy importer
nex3 Sep 7, 2023
bc4b220
Keep URL-decoding paths in includedFiles
nex3 Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 45 additions & 17 deletions lib/src/compile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,32 @@ import {MessageTransformer} from './message-transformer';
import {PacketTransformer} from './packet-transformer';
import {SyncEmbeddedCompiler} from './sync-compiler';
import {deprotofySourceSpan} from './deprotofy-span';
import {legacyImporterProtocol} from './legacy/importer';
import {
removeLegacyImporter,
removeLegacyImporterFromSpan,
legacyImporterProtocol,
} from './legacy/utils';

/// Allow the legacy API to pass in an option signaling to the modern API that
/// it's being run in legacy mode.
///
/// This is not intended for API users to pass in, and may be broken without
/// warning in the future.
type OptionsWithLegacy<sync extends 'sync' | 'async'> = Options<sync> & {
legacy?: boolean;
};

/// Allow the legacy API to pass in an option signaling to the modern API that
/// it's being run in legacy mode.
///
/// This is not intended for API users to pass in, and may be broken without
/// warning in the future.
type StringOptionsWithLegacy<sync extends 'sync' | 'async'> =
StringOptions<sync> & {legacy?: boolean};

export function compile(
path: string,
options?: Options<'sync'>
options?: OptionsWithLegacy<'sync'>
): CompileResult {
const importers = new ImporterRegistry(options);
return compileRequestSync(
Expand All @@ -34,7 +55,7 @@ export function compile(

export function compileString(
source: string,
options?: StringOptions<'sync'>
options?: StringOptionsWithLegacy<'sync'>
): CompileResult {
const importers = new ImporterRegistry(options);
return compileRequestSync(
Expand All @@ -46,7 +67,7 @@ export function compileString(

export function compileAsync(
path: string,
options?: Options<'async'>
options?: OptionsWithLegacy<'async'>
): Promise<CompileResult> {
const importers = new ImporterRegistry(options);
return compileRequestAsync(
Expand All @@ -58,7 +79,7 @@ export function compileAsync(

export function compileStringAsync(
source: string,
options?: StringOptions<'async'>
options?: StringOptionsWithLegacy<'async'>
): Promise<CompileResult> {
const importers = new ImporterRegistry(options);
return compileRequestAsync(
Expand Down Expand Up @@ -151,7 +172,7 @@ function newCompileRequest(
async function compileRequestAsync(
request: proto.InboundMessage_CompileRequest,
importers: ImporterRegistry<'async'>,
options?: Options<'async'>
options?: OptionsWithLegacy<'async'> & {legacy?: boolean}
): Promise<CompileResult> {
const functions = new FunctionRegistry(options?.functions);
const embeddedCompiler = new AsyncEmbeddedCompiler();
Expand Down Expand Up @@ -197,7 +218,7 @@ async function compileRequestAsync(
function compileRequestSync(
request: proto.InboundMessage_CompileRequest,
importers: ImporterRegistry<'sync'>,
options?: Options<'sync'>
options?: OptionsWithLegacy<'sync'>
): CompileResult {
const functions = new FunctionRegistry(options?.functions);
const embeddedCompiler = new SyncEmbeddedCompiler();
Expand Down Expand Up @@ -272,33 +293,40 @@ function createDispatcher<sync extends 'sync' | 'async'>(

/** Handles a log event according to `options`. */
function handleLogEvent(
options: Options<'sync' | 'async'> | undefined,
options: OptionsWithLegacy<'sync' | 'async'> | undefined,
event: proto.OutboundMessage_LogEvent
): void {
let span = event.span ? deprotofySourceSpan(event.span) : null;
if (span && options?.legacy) span = removeLegacyImporterFromSpan(span);
let message = event.message;
if (options?.legacy) message = removeLegacyImporter(message);
let formatted = event.formatted;
if (options?.legacy) formatted = removeLegacyImporter(formatted);

if (event.type === proto.LogEventType.DEBUG) {
if (options?.logger?.debug) {
options.logger.debug(event.message, {
span: deprotofySourceSpan(event.span!),
options.logger.debug(message, {
span: span!,
});
} else {
console.error(event.formatted);
console.error(formatted);
}
} else {
if (options?.logger?.warn) {
const params: {deprecation: boolean; span?: SourceSpan; stack?: string} =
{
deprecation: event.type === proto.LogEventType.DEPRECATION_WARNING,
};

const spanProto = event.span;
if (spanProto) params.span = deprotofySourceSpan(spanProto);
if (span) params.span = span;

const stack = event.stackTrace;
if (stack) params.stack = stack;
if (stack) {
params.stack = options?.legacy ? removeLegacyImporter(stack) : stack;
}

options.logger.warn(event.message, params);
options.logger.warn(message, params);
} else {
console.error(event.formatted);
console.error(formatted);
}
}
}
Expand Down
63 changes: 32 additions & 31 deletions lib/src/legacy/importer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// https://opensource.org/licenses/MIT.

import {strict as assert} from 'assert';
import {pathToFileURL, URL as NodeURL} from 'url';
import * as fs from 'fs';
import * as p from 'path';
import * as util from 'util';
Expand All @@ -26,6 +25,12 @@ import {
LegacyPluginThis,
LegacySyncImporter,
} from '../vendor/sass';
import {
pathToLegacyFileUrl,
legacyFileUrlToPath,
legacyImporterProtocol,
legacyImporterProtocolPrefix,
} from './utils';

/**
* A special URL protocol we use to signal when a stylesheet has finished
Expand All @@ -36,9 +41,9 @@ import {
export const endOfLoadProtocol = 'sass-embedded-legacy-load-done:';

/**
* The URL protocol to use for URLs canonicalized using `LegacyImporterWrapper`.
* The `file:` URL protocol with [legacyImporterProtocolPrefix] at the beginning.
*/
export const legacyImporterProtocol = 'legacy-importer:';
export const legacyImporterFileProtocol = 'legacy-importer-file:';

// A count of `endOfLoadProtocol` imports that have been generated. Each one
// must be a different URL to ensure that the importer results aren't cached.
Expand Down Expand Up @@ -68,11 +73,6 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
// `this.callbacks`, if it returned one.
private lastContents: string | undefined;

// Whether we're expecting the next call to `canonicalize()` to be a relative
// load. The legacy importer API doesn't handle these loads in the same way as
// the modern API, so we always return `null` in this case.
private expectingRelativeLoad = true;

constructor(
private readonly self: LegacyPluginThis,
private readonly callbacks: Array<LegacyImporter<sync>>,
Expand All @@ -90,14 +90,23 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
): PromiseOr<URL | null, sync> {
if (url.startsWith(endOfLoadProtocol)) return new URL(url);

// Since there's only ever one modern importer in legacy mode, we can be
// sure that all normal loads are preceded by exactly one relative load.
if (this.expectingRelativeLoad) {
if (url.startsWith('file:')) {
if (
url.startsWith(legacyImporterProtocolPrefix) ||
url.startsWith(legacyImporterProtocol)
) {
// A load starts with `legacyImporterProtocolPrefix` if and only if it's a
// relative load for the current importer rather than an absolute load.
// For the most part, we want to ignore these, but for `file:` URLs
// specifically we want to resolve them on the filesystem to ensure
// locality.
const urlWithoutPrefix = url.substring(
legacyImporterProtocolPrefix.length
);
if (urlWithoutPrefix.startsWith('file:')) {
let resolved: string | null = null;

try {
const path = fileUrlToPathCrossPlatform(url);
const path = fileUrlToPathCrossPlatform(urlWithoutPrefix);
resolved = resolvePath(path, options.fromImport);
} catch (error: unknown) {
if (
Expand All @@ -119,16 +128,11 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>

if (resolved !== null) {
this.prev.push({url: resolved, path: true});
return pathToFileURL(resolved);
return pathToLegacyFileUrl(resolved);
}
}

this.expectingRelativeLoad = false;
return null;
} else if (!url.startsWith('file:')) {
// We'll only search for another relative import when the URL isn't
// absolute.
this.expectingRelativeLoad = true;
}

const prev = this.prev[this.prev.length - 1];
Expand All @@ -153,14 +157,14 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
encodeURI((result as {file: string}).file)
);
} else if (/^[A-Za-z+.-]+:/.test(url)) {
return new URL(url);
return new URL(`${legacyImporterProtocolPrefix}${url}`);
} else {
return new URL(legacyImporterProtocol + encodeURI(url));
}
} else {
if (p.isAbsolute(result.file)) {
const resolved = resolvePath(result.file, options.fromImport);
return resolved ? pathToFileURL(resolved) : null;
return resolved ? pathToLegacyFileUrl(resolved) : null;
}

const prefixes = [...this.loadPaths, '.'];
Expand All @@ -171,16 +175,16 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
p.join(prefix, result.file),
options.fromImport
);
if (resolved !== null) return pathToFileURL(resolved);
if (resolved !== null) return pathToLegacyFileUrl(resolved);
}
return null;
}
}),
result => {
if (result !== null) {
const path = result.protocol === 'file:';
const path = result.protocol === legacyImporterFileProtocol;
this.prev.push({
url: path ? fileUrlToPathCrossPlatform(result as NodeURL) : url,
url: path ? legacyFileUrlToPath(result) : url,
path,
});
return result;
Expand All @@ -190,7 +194,7 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
p.join(loadPath, url),
options.fromImport
);
if (resolved !== null) return pathToFileURL(resolved);
if (resolved !== null) return pathToLegacyFileUrl(resolved);
}
return null;
}
Expand All @@ -208,7 +212,7 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
};
}

if (canonicalUrl.protocol === 'file:') {
if (canonicalUrl.protocol === legacyImporterFileProtocol) {
const syntax = canonicalUrl.pathname.endsWith('.sass')
? 'indented'
: canonicalUrl.pathname.endsWith('.css')
Expand All @@ -217,10 +221,7 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>

let contents =
this.lastContents ??
fs.readFileSync(
fileUrlToPathCrossPlatform(canonicalUrl as NodeURL),
'utf-8'
);
fs.readFileSync(legacyFileUrlToPath(canonicalUrl), 'utf-8');
this.lastContents = undefined;
if (syntax === 'scss') {
contents += this.endOfLoadImport;
Expand Down Expand Up @@ -311,7 +312,7 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
// The `@import` statement to inject after the contents of files to ensure
// that we know when a load has completed so we can pass the correct `prev`
// argument to callbacks.
private get endOfLoadImport() {
private get endOfLoadImport(): string {
return `\n;@import "${endOfLoadProtocol}${endOfLoadCount++}";`;
}
}
Loading
Loading