Skip to content

Commit

Permalink
Fix JavaScript dependencies in bundles. (denoland#4215)
Browse files Browse the repository at this point in the history
Fixes denoland#4602

We turned off `allowJs` by default, to keep the compiler from grabbing
a bunch of files that it wouldn't actually do anything useful with.  On
the other hand, this caused problems with bundles, where the compiler
needs to gather all the dependencies, including JavaScript ones.  This
fixes this so that when we are bundling, we analyse JavaScript imports
in the compiler.
  • Loading branch information
kitsonk committed Mar 2, 2020
1 parent a3c3a56 commit 83d902a
Show file tree
Hide file tree
Showing 6 changed files with 82 additions and 23 deletions.
49 changes: 34 additions & 15 deletions cli/js/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ async function tsCompilerOnMessage({
const resolvedRootModules = await processImports(
rootNames.map(rootName => [rootName, rootName]),
undefined,
host.getCompilationSettings().checkJs
bundle || host.getCompilationSettings().checkJs
);

let emitSkipped = true;
Expand Down Expand Up @@ -208,27 +208,46 @@ async function tsCompilerOnMessage({
? rootName
: resolveModules([rootName])[0];

// recursively process imports, loading each file into memory. If there
// are sources, these files are pulled out of the there, otherwise the
// files are retrieved from the privileged side
const rootNames = sources
? processLocalImports(sources, [[resolvedRootName, resolvedRootName]])
: await processImports([[resolvedRootName, resolvedRootName]]);

// if there are options, convert them into TypeScript compiler options,
// and resolve any external file references
let convertedOptions: ts.CompilerOptions | undefined;
let additionalFiles: string[] | undefined;
if (options) {
const result = convertCompilerOptions(options);
convertedOptions = result.options;
if (result.files) {
// any files supplied in the configuration are resolved externally,
// even if sources are provided
const resolvedNames = resolveModules(result.files);
rootNames.push(
...(await processImports(resolvedNames.map(rn => [rn, rn])))
additionalFiles = result.files;
}

const checkJsImports =
bundle || (convertedOptions && convertedOptions.checkJs);

// recursively process imports, loading each file into memory. If there
// are sources, these files are pulled out of the there, otherwise the
// files are retrieved from the privileged side
const rootNames = sources
? processLocalImports(
sources,
[[resolvedRootName, resolvedRootName]],
undefined,
checkJsImports
)
: await processImports(
[[resolvedRootName, resolvedRootName]],
undefined,
checkJsImports
);
}

if (additionalFiles) {
// any files supplied in the configuration are resolved externally,
// even if sources are provided
const resolvedNames = resolveModules(additionalFiles);
rootNames.push(
...(await processImports(
resolvedNames.map(rn => [rn, rn]),
undefined,
checkJsImports
))
);
}

const state: WriteFileState = {
Expand Down
9 changes: 9 additions & 0 deletions cli/js/compiler_api_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,15 @@ test(async function bundleApiConfig() {
assert(!actual.includes(`random`));
});

test(async function bundleApiJsModules() {
const [diagnostics, actual] = await bundle("/foo.js", {
"/foo.js": `export * from "./bar.js";\n`,
"/bar.js": `export const bar = "bar";\n`
});
assert(diagnostics == null);
assert(actual.includes(`System.register("bar",`));
});

test(async function diagnosticsTest() {
const [diagnostics] = await compile("/foo.ts", {
"/foo.ts": `document.getElementById("foo");`
Expand Down
1 change: 1 addition & 0 deletions cli/js/compiler_host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export interface ConfigureResponse {
/** Options that need to be used when generating a bundle (either trusted or
* runtime). */
export const defaultBundlerOptions: ts.CompilerOptions = {
allowJs: true,
inlineSourceMap: false,
module: ts.ModuleKind.System,
outDir: undefined,
Expand Down
12 changes: 6 additions & 6 deletions cli/js/compiler_imports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ export function processLocalImports(
sources: Record<string, string>,
specifiers: Array<[string, string]>,
referrer?: string,
checkJs = false
processJsImports = false
): string[] {
if (!specifiers.length) {
return [];
Expand All @@ -145,9 +145,9 @@ export function processLocalImports(
if (!sourceFile.processed) {
processLocalImports(
sources,
sourceFile.imports(checkJs),
sourceFile.imports(processJsImports),
sourceFile.url,
checkJs
processJsImports
);
}
}
Expand All @@ -163,7 +163,7 @@ export function processLocalImports(
export async function processImports(
specifiers: Array<[string, string]>,
referrer?: string,
checkJs = false
processJsImports = false
): Promise<string[]> {
if (!specifiers.length) {
return [];
Expand All @@ -179,9 +179,9 @@ export async function processImports(
sourceFile.cache(specifiers[i][0], referrer);
if (!sourceFile.processed) {
await processImports(
sourceFile.imports(checkJs),
sourceFile.imports(processJsImports),
sourceFile.url,
checkJs
processJsImports
);
}
}
Expand Down
4 changes: 2 additions & 2 deletions cli/js/compiler_sourcefile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export class SourceFile {
}

/** Process the imports for the file and return them. */
imports(checkJs: boolean): Array<[string, string]> {
imports(processJsImports: boolean): Array<[string, string]> {
if (this.processed) {
throw new Error("SourceFile has already been processed.");
}
Expand Down Expand Up @@ -134,7 +134,7 @@ export class SourceFile {
}
} else if (
!(
!checkJs &&
!processJsImports &&
(this.mediaType === MediaType.JavaScript ||
this.mediaType === MediaType.JSX)
)
Expand Down
30 changes: 30 additions & 0 deletions cli/tests/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,36 @@ fn bundle_tla() {
assert_eq!(output.stderr, b"");
}

#[test]
fn bundle_js() {
use tempfile::TempDir;

// First we have to generate a bundle of some module that has exports.
let mod6 = util::root_path().join("cli/tests/subdir/mod6.js");
assert!(mod6.is_file());
let t = TempDir::new().expect("tempdir fail");
let bundle = t.path().join("mod6.bundle.js");
let mut deno = util::deno_cmd()
.current_dir(util::root_path())
.arg("bundle")
.arg(mod6)
.arg(&bundle)
.spawn()
.expect("failed to spawn script");
let status = deno.wait().expect("failed to wait for the child process");
assert!(status.success());
assert!(bundle.is_file());

let output = util::deno_cmd()
.current_dir(util::root_path())
.arg("run")
.arg(&bundle)
.output()
.expect("failed to spawn script");
// check that nothing went to stderr
assert_eq!(output.stderr, b"");
}

#[test]
fn repl_test_console_log() {
let (out, err, code) = util::run_and_collect_output(
Expand Down

0 comments on commit 83d902a

Please sign in to comment.