Skip to content

Commit

Permalink
fix #689 and #730: add cwd option, disable service stop()
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Feb 1, 2021
1 parent 2681556 commit a2a221e
Show file tree
Hide file tree
Showing 13 changed files with 219 additions and 137 deletions.
12 changes: 12 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,17 @@
# Changelog

## Unreleased

* Fix the JavaScript watch mode API exiting early ([#730](https://github.com/evanw/esbuild/issues/730))

The previous release contained a bug that caused the JavaScript watch mode API to exit early in some cases. This bug should now be fixed. The problem was caused by some code that shouldn't even need to exist now that you are no longer required to call `stop()` on an esbuild service created by `startService()` (it was made optional in version 0.8.32). I took the opportunity to clean up the internals of esbuild's JavaScript API implementation which ended up removing the entire section of code that contained this bug.

* Add an API option for a per-build working directory ([#689](https://github.com/evanw/esbuild/issues/689))

You can now use the `absWorkingDir` API option to customize the current working directory. It will default to the value of `process.cwd()` at the time of the call to `startService()` when not specified, which matches the existing behavior. The working directory is used for a few different things including resolving relative paths given as API options to absolute paths and pretty-printing absolute paths as relative paths in log messages.

In addition to being a useful feature, this change also simplifies esbuild's internals. Previously esbuild had to maintain separate child processes if the current working directory was changed in between build API calls. Now esbuild will always reuse the same child process across all build API calls. The `stop()` call on the `startService()` API is also now a no-op (it doesn't do anything anymore) and the `startService()` API may be removed in future releases.

## 0.8.38

* Implement a simple cross-platform watch mode ([#21](https://github.com/evanw/esbuild/issues/21))
Expand Down
1 change: 1 addition & 0 deletions cmd/esbuild/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ func (service *serviceType) handleBuildRequest(id uint32, request map[string]int
flags := decodeStringArray(request["flags"].([]interface{}))

options, err := cli.ParseBuildOptions(flags)
options.AbsWorkingDir = request["absWorkingDir"].(string)

// Normally when "write" is true and there is no output file/directory then
// the output is written to stdout instead. However, we're currently using
Expand Down
55 changes: 32 additions & 23 deletions internal/fs/fs_real.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package fs

import (
"fmt"
"io/ioutil"
"os"
"sort"
Expand Down Expand Up @@ -49,9 +50,10 @@ type privateWatchData struct {

type RealFSOptions struct {
WantWatchData bool
AbsWorkingDir string
}

func RealFS(options RealFSOptions) FS {
func RealFS(options RealFSOptions) (FS, error) {
var fp goFilepath
if checkIfWindows() {
fp.isWindows = true
Expand All @@ -61,29 +63,36 @@ func RealFS(options RealFSOptions) FS {
fp.pathSeparator = '/'
}

if cwd, err := os.Getwd(); err != nil {
// This probably only happens in the browser
fp.cwd = "/"
} else {
// Resolve symlinks in the current working directory. Symlinks are resolved
// when input file paths are converted to absolute paths because we need to
// recognize an input file as unique even if it has multiple symlinks
// pointing to it. The build will generate relative paths from the current
// working directory to the absolute input file paths for error messages,
// so the current working directory should be processed the same way. Not
// doing this causes test failures with esbuild when run from inside a
// symlinked directory.
//
// This deliberately ignores errors due to e.g. infinite loops. If there is
// an error, we will just use the original working directory and likely
// encounter an error later anyway. And if we don't encounter an error
// later, then the current working directory didn't even matter and the
// error is unimportant.
if path, err := fp.evalSymlinks(cwd); err == nil {
fp.cwd = path
} else {
// Come up with a default working directory if one was not specified
fp.cwd = options.AbsWorkingDir
if fp.cwd == "" {
if cwd, err := os.Getwd(); err == nil {
fp.cwd = cwd
} else if fp.isWindows {
fp.cwd = "C:\\"
} else {
fp.cwd = "/"
}
} else if !fp.isAbs(fp.cwd) {
return nil, fmt.Errorf("The working directory %q is not an absolute path", fp.cwd)
}

// Resolve symlinks in the current working directory. Symlinks are resolved
// when input file paths are converted to absolute paths because we need to
// recognize an input file as unique even if it has multiple symlinks
// pointing to it. The build will generate relative paths from the current
// working directory to the absolute input file paths for error messages,
// so the current working directory should be processed the same way. Not
// doing this causes test failures with esbuild when run from inside a
// symlinked directory.
//
// This deliberately ignores errors due to e.g. infinite loops. If there is
// an error, we will just use the original working directory and likely
// encounter an error later anyway. And if we don't encounter an error
// later, then the current working directory didn't even matter and the
// error is unimportant.
if path, err := fp.evalSymlinks(fp.cwd); err == nil {
fp.cwd = path
}

// Only allocate memory for watch data if necessary
Expand All @@ -96,7 +105,7 @@ func RealFS(options RealFSOptions) FS {
entries: make(map[string]entriesOrErr),
fp: fp,
watchData: watchData,
}
}, nil
}

func (fs *realFS) ReadDirectory(dir string) (map[string]*Entry, error) {
Expand Down
5 changes: 3 additions & 2 deletions lib/browser.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const transformSync: typeof types.transformSync = () => {
throw new Error(`The "transformSync" API only works in node`);
};

export const startService: typeof types.startService = common.referenceCountedService(() => '', async (options) => {
export const startService: typeof types.startService = common.longLivedService(() => '', async (options) => {
if (!options) throw new Error('Must provide an options object to "startService"');
options = common.validateServiceOptions(options)!;
let wasmURL = options.wasmURL;
Expand Down Expand Up @@ -80,7 +80,7 @@ export const startService: typeof types.startService = common.referenceCountedSe
return {
build: (options: types.BuildOptions): Promise<any> =>
new Promise<types.BuildResult>((resolve, reject) =>
service.buildOrServe('build', null, null, options, false, (err, res) =>
service.buildOrServe('build', null, null, options, false, '/', (err, res) =>
err ? reject(err) : resolve(res as types.BuildResult))),
transform: (input, options) => {
input += '';
Expand All @@ -100,6 +100,7 @@ export const startService: typeof types.startService = common.referenceCountedSe
throw new Error(`The "transformSync" API only works in node`);
},
stop() {
// Note: This is now never called
worker.terminate()
afterClose()
},
Expand Down
102 changes: 57 additions & 45 deletions lib/common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ function flagsForBuildOptions(
plugins: types.Plugin[] | undefined,
stdinContents: string | null,
stdinResolveDir: string | null,
absWorkingDir: string | undefined,
incremental: boolean,
watch: types.WatchMode | null,
} {
Expand Down Expand Up @@ -185,6 +186,7 @@ function flagsForBuildOptions(
let publicPath = getFlag(options, keys, 'publicPath', mustBeString);
let inject = getFlag(options, keys, 'inject', mustBeArray);
let entryPoints = getFlag(options, keys, 'entryPoints', mustBeArray);
let absWorkingDir = getFlag(options, keys, 'absWorkingDir', mustBeString);
let stdin = getFlag(options, keys, 'stdin', mustBeObject);
let write = getFlag(options, keys, 'write', mustBeBoolean) ?? writeDefault; // Default to true if not specified
let incremental = getFlag(options, keys, 'incremental', mustBeBoolean) === true;
Expand Down Expand Up @@ -273,6 +275,7 @@ function flagsForBuildOptions(
plugins,
stdinContents,
stdinResolveDir,
absWorkingDir,
incremental,
watch: watchMode,
};
Expand Down Expand Up @@ -333,6 +336,7 @@ export interface StreamService {
serveOptions: types.ServeOptions | null,
options: types.BuildOptions,
isTTY: boolean,
defaultWD: string,
callback: (err: Error | null, res: types.BuildResult | types.ServeResult | null) => void,
): void;

Expand Down Expand Up @@ -733,7 +737,7 @@ export function createChannel(streamIn: StreamIn): StreamOut {
afterClose,

service: {
buildOrServe(callName, callerRefs, serveOptions, options, isTTY, callback) {
buildOrServe(callName, callerRefs, serveOptions, options, isTTY, defaultWD, callback) {
let pluginRefs: Refs | undefined;
const details = createObjectStash();
const logLevelDefault = 'info';
Expand All @@ -756,6 +760,7 @@ export function createChannel(streamIn: StreamIn): StreamOut {
plugins,
stdinContents,
stdinResolveDir,
absWorkingDir,
incremental,
watch,
} = flagsForBuildOptions(callName, options, isTTY, logLevelDefault, writeDefault);
Expand All @@ -766,6 +771,7 @@ export function createChannel(streamIn: StreamIn): StreamOut {
write,
stdinContents,
stdinResolveDir,
absWorkingDir: absWorkingDir || defaultWD,
incremental,
hasOnRebuild: !!(watch && watch.onRebuild),
};
Expand Down Expand Up @@ -854,6 +860,9 @@ export function createChannel(streamIn: StreamIn): StreamOut {
if (serve) {
let serveResponse = response as any as protocol.ServeResponse;
let isStopped = false

// Add a ref/unref for "stop()"
refs.ref()
let result: types.ServeResult = {
port: serveResponse.port,
host: serveResponse.host,
Expand All @@ -865,7 +874,15 @@ export function createChannel(streamIn: StreamIn): StreamOut {
refs.unref() // Do this after the callback so "stop" can extend the lifetime
},
};

// Add a ref/unref for "wait". This must be done independently of
// "stop()" in case the response to "stop()" comes in first before
// the request for "wait". Without this ref/unref, node may close
// the child's stdin pipe after the "stop()" but before the "wait"
// which will cause things to break. This caused a test failure.
refs.ref()
serve.wait.then(refs.unref, refs.unref)

return callback(null, result);
}
return buildResponseToResult(response!, callback);
Expand Down Expand Up @@ -1120,76 +1137,71 @@ function convertOutputFiles({ path, contents }: protocol.BuildOutputFile): types
}
}

export function referenceCountedService(getwd: () => string, startService: typeof types.startService): typeof types.startService {
interface Entry {
promise: Promise<types.Service>;
refCount: number;
}

let entries = new Map<string, Entry>();

// This function serves two purposes:
//
// a) Only create one long-lived service for each unique value of "options".
// This is useful because creating a service is expensive and it's
// sometimes convenient for multiple independent libraries to create
// esbuild services without coordinating with each other. This pooling
// optimization makes this use case efficient.
//
// b) Set the default working directory to the value of the current working
// directory at the time "startService()" was called. This means each call
// to "startService()" can potentially have a different default working
// directory.
//
// TODO: This is legacy behavior that originated because "startService()"
// used to correspond to creating a new child process. That is no longer
// the case because child processes are now pooled. This behavior is
// being preserved for compatibility with Snowpack for now. I would like
// to remove this strange behavior in a future release now that we have
// the "absWorkingDir" API option.
//
export function longLivedService(getwd: () => string, startService: typeof types.startService): typeof types.startService {
let entries = new Map<string, Promise<types.Service>>();
return async (options) => {
// Mix the current working directory into the key. Some users rely on
// calling "process.chdir()" before calling "startService()" to set the
// current working directory for that service.
let cwd = getwd();
let optionsJSON = JSON.stringify(options || {});
let key = `${optionsJSON} ${cwd}`;
let key = optionsJSON;
let entry = entries.get(key);
let didStop = false;

let checkWasStopped = () => {
if (didStop) {
throw new Error('The service was stopped');
}
};

// Returns true if this was the last reference
let isLastStop = () => {
if (!didStop) {
didStop = true;
if (--entry!.refCount === 0) {
entries.delete(key);
return true;
}
}
return false;
};

if (entry === void 0) {
// Store the promise used to create the service so that multiple
// concurrent calls to "startService()" will share the same promise.
entry = { promise: startService(JSON.parse(optionsJSON)), refCount: 0 };
entry = startService(JSON.parse(optionsJSON));
entries.set(key, entry);
}

++entry.refCount;

try {
let service = await entry.promise;
let service = await entry;
return {
build: (options: any): any => {
checkWasStopped();
build: (options: any = {}): any => {
if (cwd) {
let absWorkingDir = options.absWorkingDir
if (!absWorkingDir) options = { ...options, absWorkingDir: cwd }
}
return service.build(options);
},
serve(serveOptions, buildOptions) {
checkWasStopped();
serve(serveOptions, buildOptions: any = {}) {
if (cwd) {
let absWorkingDir = buildOptions.absWorkingDir
if (!absWorkingDir) buildOptions = { ...buildOptions, absWorkingDir: cwd }
}
return service.serve(serveOptions, buildOptions);
},
transform(input, options) {
checkWasStopped();
return service.transform(input, options);
},
stop() {
if (isLastStop()) {
service.stop();
}
// This is now a no-op
},
};
}

catch (e) {
isLastStop();
// Remove the entry if loading fails, which allows
// us to try again (only happens in the browser)
entries.delete(key);
throw e;
}
};
Expand Down
Loading

0 comments on commit a2a221e

Please sign in to comment.