-
Notifications
You must be signed in to change notification settings - Fork 29
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
[Shared Resources] Embedded host implementation #261
Merged
Merged
Changes from 5 commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
e8032ae
Flesh out the sync Compiler class
jerivas eb23204
Factor out common compilation helpers
jerivas 9e78051
Add AsyncCompiler
jerivas 4aaa65f
Autofix
jerivas 1caf32e
Bring back legacy checks
jerivas b41959e
Don't autoclose the sync compiler
jerivas 7906f99
Update module exports
jerivas ebfddd9
Let active compilations settle before disposing
jerivas c47a958
update copyright
jgerigmeyer a6ca5d9
Add try/catch to sync functions as well
jerivas 8ecb0e3
Export compiler classes
jerivas ba7c156
Remove complete compilations from compiler state
jerivas cdd32ca
Update comments
jerivas fbbe407
Add unique compilation IDs
jerivas c35c965
Lint
jerivas 298daab
Add missing getter
jerivas 06974e1
Reuse package and message transformers
jerivas 7b7fbf7
Throw errors if Compiler classes directly contructed
jamesnw e7e4868
Merge branch 'main' of https://github.com/sass/embedded-host-node int…
jamesnw 297cf67
Use compiler-level compilation IDs
jerivas b03ef75
Fix compilation ID reset bug in AsyncCompiler and Compiler
jerivas ddfb710
Track active compilations when building the request
jerivas bf7cf3c
Update comments and type annotations
jerivas 7138091
Unsubscribe dispatchers when they execute their callback
jerivas d79ada4
Use consistent cwd for compiler processes
jerivas 26a0338
Resolve paths before sending them to the compiler process
jerivas 55e64f2
Lint
jerivas 612db59
Add missing await
jerivas 329fa94
Clean up Dispatcher class
jerivas 2556525
Update compiler imports and file structure
jerivas f5c2e8a
Update copyright year
jerivas 66a7f66
Add comments
jerivas 55a55ed
Update comment
jerivas 0f0b5aa
Merge branch 'main' into feature.shared-resources
jgerigmeyer 2b24ec4
update comments
jgerigmeyer a222ff2
copyrights are inconsistent in this repo
jgerigmeyer 84ae6a4
Add private id to FunctionRegistry
jerivas 82819b0
Merge branch 'main' into feature.shared-resources
jerivas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -6,42 +6,136 @@ import {spawn} from 'child_process'; | |||||
import {Observable} from 'rxjs'; | ||||||
import {takeUntil} from 'rxjs/operators'; | ||||||
|
||||||
import { | ||||||
OptionsWithLegacy, | ||||||
StringOptionsWithLegacy, | ||||||
createDispatcher, | ||||||
handleCompileResponse, | ||||||
handleLogEvent, | ||||||
newCompilePathRequest, | ||||||
newCompileStringRequest, | ||||||
} from './compiler'; | ||||||
import {compilerCommand} from './compiler-path'; | ||||||
import {FunctionRegistry} from './function-registry'; | ||||||
import {ImporterRegistry} from './importer-registry'; | ||||||
import * as utils from './utils'; | ||||||
import * as proto from './vendor/embedded_sass_pb'; | ||||||
import {CompileResult} from './vendor/sass'; | ||||||
|
||||||
/** | ||||||
* An asynchronous wrapper for the embedded Sass compiler that exposes its stdio | ||||||
* streams as Observables. | ||||||
* An asynchronous wrapper for the embedded Sass compiler | ||||||
*/ | ||||||
export class AsyncEmbeddedCompiler { | ||||||
export class AsyncCompiler { | ||||||
/** The underlying process that's being wrapped. */ | ||||||
private readonly process = spawn( | ||||||
compilerCommand[0], | ||||||
[...compilerCommand.slice(1), '--embedded'], | ||||||
{windowsHide: true} | ||||||
); | ||||||
|
||||||
/** Whether the underlying compiler has already exited. */ | ||||||
private disposed = false; | ||||||
|
||||||
/** The child process's exit event. */ | ||||||
readonly exit$ = new Promise<number | null>(resolve => { | ||||||
private readonly exit$ = new Promise<number | null>(resolve => { | ||||||
this.process.on('exit', code => resolve(code)); | ||||||
}); | ||||||
|
||||||
/** The buffers emitted by the child process's stdout. */ | ||||||
readonly stdout$ = new Observable<Buffer>(observer => { | ||||||
private readonly stdout$ = new Observable<Buffer>(observer => { | ||||||
this.process.stdout.on('data', buffer => observer.next(buffer)); | ||||||
}).pipe(takeUntil(this.exit$)); | ||||||
|
||||||
/** The buffers emitted by the child process's stderr. */ | ||||||
readonly stderr$ = new Observable<Buffer>(observer => { | ||||||
private readonly stderr$ = new Observable<Buffer>(observer => { | ||||||
this.process.stderr.on('data', buffer => observer.next(buffer)); | ||||||
}).pipe(takeUntil(this.exit$)); | ||||||
|
||||||
/** Writes `buffer` to the child process's stdin. */ | ||||||
writeStdin(buffer: Buffer): void { | ||||||
private writeStdin(buffer: Buffer): void { | ||||||
this.process.stdin.write(buffer); | ||||||
} | ||||||
|
||||||
private throwIfDisposed(): void { | ||||||
jerivas marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (this.disposed) { | ||||||
throw utils.compilerError('Sync compiler has already exited.'); | ||||||
} | ||||||
} | ||||||
|
||||||
// Spins up a compiler, then sends it a compile request. Returns a promise that | ||||||
// resolves with the CompileResult. Throws if there were any protocol or | ||||||
// compilation errors. Shuts down the compiler after compilation. | ||||||
private async compileRequestAsync( | ||||||
request: proto.InboundMessage_CompileRequest, | ||||||
importers: ImporterRegistry<'async'>, | ||||||
options?: OptionsWithLegacy<'async'> & {legacy?: boolean} | ||||||
): Promise<CompileResult> { | ||||||
const functions = new FunctionRegistry(options?.functions); | ||||||
this.stderr$.subscribe(data => process.stderr.write(data)); | ||||||
|
||||||
const dispatcher = createDispatcher<'async'>( | ||||||
this.stdout$, | ||||||
buffer => { | ||||||
this.writeStdin(buffer); | ||||||
}, | ||||||
{ | ||||||
handleImportRequest: request => importers.import(request), | ||||||
handleFileImportRequest: request => importers.fileImport(request), | ||||||
handleCanonicalizeRequest: request => importers.canonicalize(request), | ||||||
handleFunctionCallRequest: request => functions.call(request), | ||||||
} | ||||||
); | ||||||
|
||||||
dispatcher.logEvents$.subscribe(event => handleLogEvent(options, event)); | ||||||
|
||||||
return handleCompileResponse( | ||||||
await new Promise<proto.OutboundMessage_CompileResponse>( | ||||||
(resolve, reject) => | ||||||
dispatcher.sendCompileRequest(request, (err, response) => { | ||||||
if (err) { | ||||||
reject(err); | ||||||
} else { | ||||||
resolve(response!); | ||||||
} | ||||||
}) | ||||||
) | ||||||
); | ||||||
} | ||||||
|
||||||
compileAsync( | ||||||
path: string, | ||||||
options?: OptionsWithLegacy<'async'> | ||||||
): Promise<CompileResult> { | ||||||
this.throwIfDisposed(); | ||||||
const importers = new ImporterRegistry(options); | ||||||
return this.compileRequestAsync( | ||||||
newCompilePathRequest(path, importers, options), | ||||||
importers, | ||||||
options | ||||||
); | ||||||
} | ||||||
|
||||||
compileStringAsync( | ||||||
source: string, | ||||||
options?: StringOptionsWithLegacy<'async'> | ||||||
): Promise<CompileResult> { | ||||||
this.throwIfDisposed(); | ||||||
const importers = new ImporterRegistry(options); | ||||||
return this.compileRequestAsync( | ||||||
newCompileStringRequest(source, importers, options), | ||||||
importers, | ||||||
options | ||||||
); | ||||||
} | ||||||
|
||||||
/** Kills the child process, cleaning up all associated Observables. */ | ||||||
close() { | ||||||
async dispose() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All functions should have declared return types:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in bf7cf3c |
||||||
this.disposed = true; | ||||||
this.process.stdin.end(); | ||||||
await this.exit$; | ||||||
jamesnw marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
} | ||||||
|
||||||
export async function initAsyncCompiler(): Promise<AsyncCompiler> { | ||||||
return new AsyncCompiler(); | ||||||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more bug. New compilations can crash compiler after
cwd
ofdart-sass
process is deleted. This bug actually existed previously, but could not be exploited, because the compiler would be closed before we could remove or changecwd
.Reproduction:
Crash output:
The
cwd
of embedded compiler does not affect how imports are resolved, but it can crash dart vm if thecwd
is removed. The fix is to change thecwd
ofdart-sass
process to something that can be accessed and is unlikely to be deleted by user (e.g. directory containing the dart-sass binary):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like if you delete the CWD out from under a process you're asking for trouble to begin with, but since this looks like an easy fix we might as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in d79ada4