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

chore: remove deps from angularcli to fully get rid of tslint #1057

Merged
merged 5 commits into from
Dec 10, 2021

Conversation

Cammisuli
Copy link
Member

What it does

Removes imports to @angular/cli from the code base so that we can fully get rid of tslint.

This requires using @nrwl/tao instead to get information from generators and executors.

@Cammisuli Cammisuli marked this pull request as draft March 31, 2021 21:47
@nx-cloud
Copy link

nx-cloud bot commented Mar 31, 2021

Nx Cloud Report

CI ran the following commands for commit f51584a. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --all --parallel --target=build
#000000 nx run-many --all --parallel --target=test
#000000 nx run-many --all --parallel --target=lint

Sent with 💌 from NxCloud.

@Cammisuli Cammisuli marked this pull request as ready for review April 1, 2021 15:57
@Cammisuli Cammisuli requested a review from sandikbarr April 1, 2021 15:58
Copy link
Contributor

@sandikbarr sandikbarr left a comment

Choose a reason for hiding this comment

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

  • I think that option filtering on Arrays without items should be in the normalizeSchema/schemaToOptions instead of the UI form
  • I don't quite get the OptionPropertyDescription. Can you explain that to me a bit?
  • And just some other comments I had out loud while thinking this thru

@@ -1,9 +1,29 @@
import { Option as CliOption, OptionType } from '@angular/cli/models/interface';
import { Schema } from '@nrwl/tao/src/shared/params';

Copy link
Contributor

Choose a reason for hiding this comment

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

wonders if Nx should actually define an OptionType enum in the PropertyDescription type
...but it doesn't right now, so oh well.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, it doesn't. But that doesn't stop us from getting this type from the schema 😉

libs/schema/src/index.ts Show resolved Hide resolved
alias?: string;
hidden?: boolean;
deprecated?: boolean | string;
} & OptionPropertyDescription;

export interface Option extends Omit<CliOption, 'default'> {
tooltip?: string;
itemTooltips?: ItemTooltips;
items?: string[] | ItemsWithEnum;
Copy link
Contributor

Choose a reason for hiding this comment

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

noting that Angular's Option had aliases, but Nx/tao PropertyDescription has alias. Angular's Option also has the positional property.

Bah, this Schema stuff is already complex.

};
const getSchema = async (
options: any[],
Copy link
Contributor

Choose a reason for hiding this comment

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

It does maybe feel like maybe Nx/tao should be exporting more than just the Schema type so we could use Properties. I can live with this, just commenting.

@@ -321,3 +320,26 @@ export function toLegacyWorkspaceFormat(w: any) {
}
return w;
}

function schemaToOptions(schema: Schema): CliOption[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion to rename (acc, curr) to (cliOptions, optionName) to make this slightly more readable

option.items &&
(option.items as string[]).length === 0
);

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say this filtering more likely belongs in normalizeSchema/schemaToOptions

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that too, but I moved it here because I feel like this is a UI issue because we don't support those advanced array types.

I can move it 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah.. I guess it seems like a schema responsibility to me, and we'll have to figure out how to get the Schema to support that, which might be even harder now that we don't have it typed upstream.

@Cammisuli Cammisuli force-pushed the remove-deps-from-angularcli branch from 0f46f3b to 8c94b0f Compare December 9, 2021 02:04
@Cammisuli Cammisuli merged commit dfff467 into nrwl:master Dec 10, 2021
@Cammisuli Cammisuli deleted the remove-deps-from-angularcli branch December 10, 2021 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants