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.

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

Implement access tracking for containingUrl #285

Merged
merged 2 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 32 additions & 0 deletions lib/src/canonicalize-context.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2024 Google LLC. Use of this source code is governed by an
// MIT-style license that can be found in the LICENSE file or at
// https://opensource.org/licenses/MIT.

export class CanonicalizeContext {
readonly fromImport: boolean;

private readonly _containingUrl: URL | null;

get containingUrl(): URL | null {
this._containingUrlAccessed = true;
return this._containingUrl;
}

private _containingUrlAccessed = false;

/**
* Whether the `containingUrl` getter has been accessed.
*
* This is marked as public so that the importer registry can access it, but
* it's not part of the package's public API and should not be accessed by
* user code. It may be renamed or removed without warning in the future.
*/
get containingUrlAccessed(): boolean {
return this._containingUrlAccessed;
}

constructor(containingUrl: URL | null, fromImport: boolean) {
this._containingUrl = containingUrl;
this.fromImport = fromImport;
}
}
27 changes: 15 additions & 12 deletions lib/src/importer-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as p from 'path';
import {URL} from 'url';
import {inspect} from 'util';

import {CanonicalizeContext} from './canonicalize-context';
import * as utils from './utils';
import {FileImporter, Importer, Options} from './vendor/sass';
import * as proto from './vendor/embedded_sass_pb';
Expand Down Expand Up @@ -115,21 +116,22 @@ export class ImporterRegistry<sync extends 'sync' | 'async'> {
throw utils.compilerError('Unknown CanonicalizeRequest.importer_id');
}

const canonicalizeContext = new CanonicalizeContext(
request.containingUrl ? new URL(request.containingUrl) : null,
request.fromImport
);

return catchOr(
() => {
return thenOr(
importer.canonicalize(request.url, {
fromImport: request.fromImport,
containingUrl: request.containingUrl
? new URL(request.containingUrl)
: null,
}),
importer.canonicalize(request.url, canonicalizeContext),
url =>
new proto.InboundMessage_CanonicalizeResponse({
result:
url === null
? {case: undefined}
: {case: 'url', value: url.toString()},
containingUrlUnused: !canonicalizeContext.containingUrlAccessed,
})
);
},
Expand Down Expand Up @@ -197,15 +199,15 @@ export class ImporterRegistry<sync extends 'sync' | 'async'> {
throw utils.compilerError('Unknown FileImportRequest.importer_id');
}

const canonicalizeContext = new CanonicalizeContext(
request.containingUrl ? new URL(request.containingUrl) : null,
request.fromImport
);

return catchOr(
() => {
return thenOr(
importer.findFileUrl(request.url, {
fromImport: request.fromImport,
containingUrl: request.containingUrl
? new URL(request.containingUrl)
: null,
}),
importer.findFileUrl(request.url, canonicalizeContext),
url => {
if (!url) return new proto.InboundMessage_FileImportResponse();
if (url.protocol !== 'file:') {
Expand All @@ -216,6 +218,7 @@ export class ImporterRegistry<sync extends 'sync' | 'async'> {
}
return new proto.InboundMessage_FileImportResponse({
result: {case: 'fileUrl', value: url.toString()},
containingUrlUnused: !canonicalizeContext.containingUrlAccessed,
});
}
);
Expand Down
26 changes: 25 additions & 1 deletion lib/src/legacy/importer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,34 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>

canonicalize(
url: string,
options: {fromImport: boolean}
options: {fromImport: boolean; containingUrl: URL | null}
): PromiseOr<URL | null, sync> {
if (url.startsWith(endOfLoadProtocol)) return new URL(url);

// Emulate a base importer instead of using a real base importer,
// because we want to mark containingUrl as used, which is impossible
// in a real base importer.
if (options.containingUrl !== null) {
try {
const absoluteUrl = new URL(url, options.containingUrl).toString();
const resolved = this.canonicalize(absoluteUrl, {
fromImport: options.fromImport,
containingUrl: null,
});
if (resolved !== null) return resolved;
} catch (error: unknown) {
if (
error instanceof TypeError &&
isErrnoException(error) &&
error.code === 'ERR_INVALID_URL'
) {
// ignore
} else {
throw error;
}
}
}

if (
url.startsWith(legacyImporterProtocolPrefix) ||
url.startsWith(legacyImporterProtocol)
Expand Down
19 changes: 14 additions & 5 deletions lib/src/legacy/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
LegacyStringOptions,
Options,
StringOptions,
Importer,

Check warning on line 35 in lib/src/legacy/index.ts

View workflow job for this annotation

GitHub Actions / Static analysis

'Importer' is defined but never used
FileImporter,

Check warning on line 36 in lib/src/legacy/index.ts

View workflow job for this annotation

GitHub Actions / Static analysis

'FileImporter' is defined but never used
} from '../vendor/sass';
import {wrapFunction} from './value/wrap';
import {endOfLoadProtocol, LegacyImporterWrapper} from './importer';
Expand Down Expand Up @@ -184,11 +184,20 @@
): StringOptions<sync> & {legacy: true} {
const modernOptions = convertOptions(options, sync);

// Find the first non-NodePackageImporter to pass as legacy `importer` option.
// NodePackageImporter will be passed in `modernOptions.importers`.
const importer = modernOptions.importers?.find(
_importer => !(_importer instanceof NodePackageImporter)
) as Importer<sync> | FileImporter<sync>;
// Use a no-op base importer, because the LegacyImporterWrapper will emulate
// the base importer by itself in order to mark containingUrl as accessed.
const importer = modernOptions.importers?.some(
importer => importer instanceof LegacyImporterWrapper
)
? {
canonicalize() {
return null;
},
load() {
return null;
},
}
: undefined;

return {
...modernOptions,
Expand Down
8 changes: 6 additions & 2 deletions lib/src/value/argument-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,21 @@ export class SassArgumentList extends SassList {
*/
readonly keywordsInternal: OrderedMap<string, Value>;

private _keywordsAccessed = false;

/**
* Whether the `keywords` getter has been accessed.
*
* This is marked as public so that the protofier can access it, but it's not
* part of the package's public API and should not be accessed by user code.
* It may be renamed or removed without warning in the future.
*/
keywordsAccessed = false;
get keywordsAccessed(): boolean {
return this._keywordsAccessed;
}

get keywords(): OrderedMap<string, Value> {
this.keywordsAccessed = true;
this._keywordsAccessed = true;
return this.keywordsInternal;
}

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "sass-embedded",
"version": "1.75.0",
"protocol-version": "2.6.0",
"protocol-version": "2.7.0",
"compiler-version": "1.75.0",
"description": "Node.js library that communicates with Embedded Dart Sass using the Embedded Sass protocol",
"repository": "sass/embedded-host-node",
Expand Down