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

feat(js): infer tslib as a dependency when using importHelpers #9350

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
36 changes: 32 additions & 4 deletions e2e/js/src/js.test.ts
Expand Up @@ -5,7 +5,6 @@ import {
readJson,
runCLI,
runCLIAsync,
runCommand,
runCommandUntil,
uniq,
updateFile,
Expand All @@ -14,8 +13,13 @@ import {
} from '../../utils';

describe('js e2e', () => {
let scope: string;

beforeEach(() => {
scope = newProject();
});

it('should create libs with npm scripts', () => {
const scope = newProject();
const npmScriptsLib = uniq('npmscriptslib');
runCLI(`generate @nrwl/js:lib ${npmScriptsLib} --config=npm-scripts`);
const libPackageJson = readJson(`libs/${npmScriptsLib}/package.json`);
Expand All @@ -31,7 +35,6 @@ describe('js e2e', () => {
}, 120000);

it('should create libs with js executors (--compiler=tsc)', async () => {
const scope = newProject();
const lib = uniq('lib');
runCLI(`generate @nrwl/js:lib ${lib} --buildable --compiler=tsc`);
const libPackageJson = readJson(`libs/${lib}/package.json`);
Expand Down Expand Up @@ -123,10 +126,35 @@ describe('js e2e', () => {
const output = runCLI(`build ${parentLib}`);
expect(output).toContain('1 task(s) it depends on');
expect(output).toContain('Done compiling TypeScript files');

updateJson(`libs/${lib}/tsconfig.json`, (json) => {
json.compilerOptions = { ...json.compilerOptions, importHelpers: true };
return json;
});

runCLI(`build ${lib}`);

const rootPackageJson = readJson(`package.json`);

expect(readJson(`dist/libs/${lib}/package.json`)).toHaveProperty(
'peerDependencies.tslib',
rootPackageJson.dependencies.tslib ??
rootPackageJson.devDependencies.tslib
);

updateJson(`libs/${lib}/tsconfig.json`, (json) => {
json.compilerOptions = { ...json.compilerOptions, importHelpers: false };
return json;
});

runCLI(`build ${lib}`);

expect(readJson(`dist/libs/${lib}/package.json`)).not.toHaveProperty(
'peerDependencies.tslib'
);
}, 120000);

it('should create libs with js executors (--compiler=swc)', async () => {
const scope = newProject();
const lib = uniq('lib');
runCLI(`generate @nrwl/js:lib ${lib} --buildable --compiler=swc`);
const libPackageJson = readJson(`libs/${lib}/package.json`);
Expand Down
3 changes: 3 additions & 0 deletions packages/js/src/executors/tsc/tsc.impl.ts
Expand Up @@ -7,6 +7,7 @@ import { join, resolve } from 'path';
import { checkDependencies } from '../../utils/check-dependencies';
import { CopyAssetsHandler } from '../../utils/copy-assets-handler';
import { ExecutorOptions, NormalizedExecutorOptions } from '../../utils/schema';
import { addTslibDependencyIfNeeded } from '../../utils/tslib-dependency';
import { compileTypeScriptFiles } from '../../utils/typescript/compile-typescript-files';
import { updatePackageJson } from '../../utils/update-package-json';
import { watchForSingleFileChanges } from '../../utils/watch-for-single-file-changes';
Expand Down Expand Up @@ -60,6 +61,8 @@ export async function* tscExecutor(
options.tsConfig = tmpTsConfig;
}

addTslibDependencyIfNeeded(options, context, dependencies);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AgentEnder does this seem like the right approach? Perhaps there's a way to make it so that tslib will be part of the dependencies in the first place, by altering the graph somehow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine for the time being, eventually a lot of the js specific logic should be pulled out to use processProjectGraph like a community plugin would, to truly separate the core and at that point then some of this may make more sense there. Since this is specific to the tsc executor (and not wanted for the swc executor), I think this makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I figured there's something missing to do what I had in mind. Thanks for the feedback 👍🏻 I'll push a fix momentarily


const assetHandler = new CopyAssetsHandler({
projectDir: projectRoot,
rootDir: context.root,
Expand Down
47 changes: 47 additions & 0 deletions packages/js/src/utils/tslib-dependency.ts
@@ -0,0 +1,47 @@
import { ExecutorContext, getPackageManagerCommand } from '@nrwl/devkit';
import { readCachedProjectGraph } from '@nrwl/workspace/src/core/project-graph';
import { DependentBuildableProjectNode } from '@nrwl/workspace/src/utilities/buildable-libs-utils';
import { readTsConfig } from '@nrwl/workspace/src/utilities/typescript';
import { NormalizedExecutorOptions } from './schema';

const tslibNodeName = 'npm:tslib';

function shouldAddTslibDependency(
tsConfig: string,
dependencies: DependentBuildableProjectNode[]
): boolean {
if (dependencies.some((dep) => dep.name === tslibNodeName)) {
return false;
}

const config = readTsConfig(tsConfig);
return !!config.options.importHelpers;
}

export function addTslibDependencyIfNeeded(
options: NormalizedExecutorOptions,
context: ExecutorContext,
dependencies: DependentBuildableProjectNode[]
): void {
if (!shouldAddTslibDependency(options.tsConfig, dependencies)) {
return;
}

const depGraph = readCachedProjectGraph();
const tslibNode = depGraph.externalNodes[tslibNodeName];

if (!tslibNode) {
const pmc = getPackageManagerCommand();
throw new Error(
`"importHelpers" is enabled for ${context.targetName} but tslib is not installed. Use "${pmc.add} tslib" to install it.`
);
}

const tslibDependency: DependentBuildableProjectNode = {
name: tslibNodeName,
outputs: [],
node: tslibNode,
};

dependencies.push(tslibDependency);
}
9 changes: 6 additions & 3 deletions packages/workspace/src/utilities/typescript.ts
Expand Up @@ -10,7 +10,7 @@ export { getSourceNodes } from './typescript/get-source-nodes';

const normalizedAppRoot = appRootPath.replace(/\\/g, '/');

let tsModule: any;
let tsModule: typeof import('typescript');

export function readTsConfig(tsConfigPath: string) {
if (!tsModule) {
Expand All @@ -31,18 +31,21 @@ function readTsConfigOptions(tsConfigPath: string) {
if (!tsModule) {
tsModule = require('typescript');
}

const readResult = tsModule.readConfigFile(
tsConfigPath,
tsModule.sys.readFile
);

// we don't need to scan the files, we only care about options
const host = {
const host: Partial<ts.ParseConfigHost> = {
readDirectory: () => [],
fileExists: tsModule.sys.fileExists,
};

return tsModule.parseJsonConfigFileContent(
readResult.config,
host,
host as ts.ParseConfigHost,
dirname(tsConfigPath)
).options;
}
Expand Down