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(core): move tsconfig.base.json to @nrwl/js:init #14467

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

xiongemi
Copy link
Collaborator

@xiongemi xiongemi commented Jan 18, 2023

  • move tsconfig.base.json generation to @nrwl/js:init
  • also add typescript installation to @nrwl/js:init
  • call @nrwl/js:init in other plugin's init
  • since @nrwl/js:init has schema flags skipPackageJson and skipTsConfig, so add these 2 flags to all the consuming plugins' schema
  • add e2e tests in e2e/workspace-create/src/create-nx-workspace-npm.test.ts
  • remove packages/workspace/src/generators/new/files-root-app since it is the same as files-integrated-repo
  • there are a few imports like import * as ts from 'typescript'; in the code; however for package based repo, typescript is not installed initially; so i move utils functions from packages/workspace/src/utilities/typescript.ts to packages/workspace/src/utilities/ts-config.ts

Current Behavior

Expected Behavior

Related Issue(s)

Fixes #

@vercel
Copy link

vercel bot commented Jan 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 16, 2023 at 3:17PM (UTC)

@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch from 4c75e53 to cfef715 Compare January 18, 2023 18:41
@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch from cfef715 to 8593277 Compare January 18, 2023 20:21
@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch from 8593277 to 7edb2ac Compare January 18, 2023 23:39
@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch 2 times, most recently from 4039515 to b37dac2 Compare January 19, 2023 18:05
@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch from b37dac2 to 87190a9 Compare January 19, 2023 20:52
@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch from 87190a9 to b0fac00 Compare January 19, 2023 21:50
@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch from b0fac00 to a604fb6 Compare January 19, 2023 22:37
@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch from a604fb6 to 491c32d Compare January 19, 2023 23:48
@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch from 491c32d to 329981c Compare January 20, 2023 06:06
@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch from 329981c to e5bed8c Compare January 20, 2023 06:35
@xiongemi xiongemi force-pushed the feat/move-tsconfig-base-from-workspace-to-js branch from e5bed8c to 99f982f Compare January 20, 2023 07:04
packages/js/src/generators/init/schema.json Outdated Show resolved Hide resolved
packages/js/package.json Outdated Show resolved Hide resolved
packages/js/src/index.ts Outdated Show resolved Hide resolved
packages/linter/generators.ts Outdated Show resolved Hide resolved
packages/next/src/generators/application/schema.d.ts Outdated Show resolved Hide resolved
packages/workspace/src/generators/library/library.ts Outdated Show resolved Hide resolved
packages/workspace/src/utilities/ts-config.ts Show resolved Hide resolved
@@ -132,7 +132,8 @@ export async function configurationGenerator(
}
}

const initTask = initGenerator(tree, {
const initTask = await initGenerator(tree, {
js: schema.js,
Copy link
Member

Choose a reason for hiding this comment

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

This should not be here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this js passed to initGenerator is just to determine whether the typescript package needs to be installed or not. it does not create any files.

Copy link
Member

Choose a reason for hiding this comment

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

I understand, but I think it would be cleaner for it not to be here... If you think it's better to be there rather than configuration generator, then ok...

Copy link
Collaborator

@FrozenPandaz FrozenPandaz left a comment

Choose a reason for hiding this comment

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

Looking good 😄

@@ -19,12 +19,13 @@ import {
writeJson,
} from '@nrwl/devkit';
import { getImportPath } from 'nx/src/utils/path';
import { Linter, lintProjectGenerator } from '@nrwl/linter';
// nx-ignore-next-line
const { Linter } = require('@nrwl/linter'); // use require to import to avoid circular dependency
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a type? Or should it be lower down after the ensurePackage('@nrwl/linter') is called?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not a type. moved it down.

Copy link
Member

@mandarini mandarini left a comment

Choose a reason for hiding this comment

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

I am approving, but leaving my comments, too! :D

@@ -132,7 +132,8 @@ export async function configurationGenerator(
}
}

const initTask = initGenerator(tree, {
const initTask = await initGenerator(tree, {
js: schema.js,
Copy link
Member

Choose a reason for hiding this comment

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

I understand, but I think it would be cleaner for it not to be here... If you think it's better to be there rather than configuration generator, then ok...

@@ -210,7 +212,11 @@ function editRootTsConfig(tree: Tree) {
}
}

export function initGenerator(tree: Tree, schema: Schema) {
export async function initGenerator(tree: Tree, schema: Schema) {
await jsInitGenerator(tree, {
Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. But someone may call the init generator and not call the configuration generator at all, so they would not need the js... But again, if you think it makes sense here, then ok!

@xiongemi xiongemi merged commit a97212b into master Feb 16, 2023
@jaysoo jaysoo deleted the feat/move-tsconfig-base-from-workspace-to-js branch February 16, 2023 16:30
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants