feat: 🎸 edit project configuration #87

amanecer2levi · 2023-03-02T10:00:28Z

✅ Closes: 15

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: 15

What is the new behavior?

Edit project configuration

Does this PR introduce a breaking change?

[ ] Yes
[x ] No

Other information

nx-cloud · 2023-03-02T10:00:30Z

Nx Cloud Report

CI is running for commit 199e799.

📂 Click to track the progress, see the status, the terminal output, and the build insights.

Sent with 💌 from NxCloud.

apps/cli-daemon/src/app/utils/extract -value-object.ts

apps/cli-daemon/src/app/workspace/workspace.service.ts

libs/configuration/src/lib/configuration/configuration.component.html

libs/configuration/src/lib/configuration/configuration.component.ts

apps/cli-daemon/src/app/utils/extract -value-object.spec.ts

libs/configuration/src/lib/configuration/configuration.component.ts

Danieliverant

I resolved what you fixed and reviewed again, please see my previous review comments

ronnetzer · 2023-03-06T11:11:28Z

apps/cli-daemon/tsconfig.json

@@ -18,6 +18,7 @@
    "noImplicitOverride": true,
    "noPropertyAccessFromIndexSignature": true,
    "noImplicitReturns": true,
-    "noFallthroughCasesInSwitch": true
+    "noFallthroughCasesInSwitch": true,
+    "resolveJsonModule": true


why is it needed? I didnt see that you import json anywhere, if its for jest, move it to tsconfig.spec.json

im importing angular.json file.

ronnetzer · 2023-03-06T11:20:03Z

libs/configuration/src/lib/configuration/configuration.component.ts

+    this.workspaceSettingsService
+      .updateWorkspaceProjectConfiguration(
+        this.projectName,
+        this.form.value as Project
+      )
+      .subscribe();


Suggested change

this.workspaceSettingsService

.updateWorkspaceProjectConfiguration(

this.projectName,

this.form.value as Project

)

.subscribe();

this.projectName$.pipe(

take(1),

switchMap(projectName =>

this.workspaceSettingsService

.updateWorkspaceProjectConfiguration(

projectName,

this.form.value as Project

)

)

).subscribe();

then you can remove this.projectName and the tap that assigns its value

ronnetzer · 2023-03-06T11:22:29Z

libs/configuration/src/lib/configuration/configuration.component.ts

+  readonly formly$: Observable<{
+    formData: JsonObject;
+    formFields: FormlyFieldConfig[];
+  }> = combineLatest([this.formData$, this.formFields$]).pipe(
+    map(([formData, formFields]) => ({ formFields, formData }))
+  );


Suggested change

readonly formly$: Observable<{

formData: JsonObject;

formFields: FormlyFieldConfig[];

}> = combineLatest([this.formData$, this.formFields$]).pipe(

map(([formData, formFields]) => ({ formFields, formData }))

);

readonly formly$: Observable<{

formData: JsonObject;

formFields: FormlyFieldConfig[];

}> = combineLatest([this.projectConfiguration$, this.workspaceProject$]).pipe(

map(([formFields, formData]) => ({ formFields, formData }))

);

then you can remove formFields$ and formData$ subjects and ngOnInit completely

ronnetzer · 2023-03-06T11:23:28Z

libs/configuration/src/lib/configuration/configuration.component.ts

+  private readonly workspaceProject$ = this.projectName$.pipe(
+    switchMap((currentProjectName: string) =>
+      this.workspaceSettingsService.readWorkspaceProject(currentProjectName)
+    )


Suggested change

)

),

distinctUntilChanged()

ronnetzer · 2023-03-06T11:23:42Z

libs/configuration/src/lib/configuration/configuration.component.ts

+    .readWorkspaceProjectNames()
+    .pipe(
+      map((names) => names[0]),
+      tap((projectName) => (this.projectName = projectName))


Suggested change

tap((projectName) => (this.projectName = projectName))

distinctUntilChanged()

ronnetzer · 2023-03-06T11:25:21Z

libs/configuration/src/lib/configuration/configuration.component.ts

 })
-export class ConfigurationComponent {}
+export class ConfigurationComponent implements OnInit {


if observables are being subscribed in more than once place you should also add shareReplay({ bufferSize: 1, refCount: true }) operator at the end, in order to avoid multiple subscriptions to the source

✅ Closes: 15

Danieliverant

Looks good to me, just a small question - have we decided to go with Formly? why not plain reactive-forms?

Danieliverant · 2023-03-07T14:17:08Z

apps/cli-daemon/src/app/workspace/workspace.service.ts

@@ -15,10 +15,14 @@ import {
  Logger,
  NotFoundException,
 } from '@nestjs/common';
+import { JSONSchema7 } from 'json-schema';
+import { Draft07 } from 'json-schema-library';
+import * as angularSchema from 'node_modules/@angular/cli/lib/config/schema.json';


why this import is from 'node_modules' and not from @angular/cli like the others?

ronnetzer · 2023-03-11T15:52:47Z

apps/cli-daemon/src/app/workspace/workspace.service.ts

@@ -15,10 +15,14 @@ import {
  Logger,
  NotFoundException,
 } from '@nestjs/common';
+import { JSONSchema7 } from 'json-schema';
+import { Draft07 } from 'json-schema-library';
+import * as angularSchema from 'node_modules/@angular/cli/lib/config/schema.json';


This file should be read in runtime. Currently you're bundling & reading our installed angular pkg, instead you need to read the installed angular pkg in the currently active user's workspace by using node's fs module (readFile, readFileSync).

After this change you can revert the changes you made in the tsconfig file

amanecer2 force-pushed the shahar/workspace-settings-configuration branch 2 times, most recently from 1a43871 to d260f16 Compare March 2, 2023 10:23

Danieliverant reviewed Mar 2, 2023

View reviewed changes

ronnetzer requested a review from a team March 2, 2023 12:14

OmerGronich reviewed Mar 2, 2023

View reviewed changes

apps/cli-daemon/src/app/utils/extract -value-object.spec.ts Outdated Show resolved Hide resolved

OmerGronich requested changes Mar 3, 2023

View reviewed changes

amanecer2 force-pushed the shahar/workspace-settings-configuration branch 2 times, most recently from 15949c3 to ca3783a Compare March 5, 2023 11:29

Danieliverant reviewed Mar 5, 2023

View reviewed changes

OmerGronich approved these changes Mar 6, 2023

View reviewed changes

ronnetzer requested changes Mar 6, 2023

View reviewed changes

amanecer2 force-pushed the shahar/workspace-settings-configuration branch from ca3783a to 019dd07 Compare March 7, 2023 13:56

feat: 🎸 edit project configuration

199e799

✅ Closes: 15

amanecer2 force-pushed the shahar/workspace-settings-configuration branch from 019dd07 to 199e799 Compare March 7, 2023 14:15

Danieliverant reviewed Mar 7, 2023

View reviewed changes

ronnetzer reviewed Mar 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 🎸 edit project configuration #87

feat: 🎸 edit project configuration #87

amanecer2levi commented Mar 2, 2023

nx-cloud bot commented Mar 2, 2023 •

edited

Loading

Danieliverant left a comment

ronnetzer Mar 6, 2023

amanecer2levi Mar 7, 2023

ronnetzer Mar 6, 2023

ronnetzer Mar 6, 2023

ronnetzer Mar 6, 2023

ronnetzer Mar 6, 2023

ronnetzer Mar 6, 2023

Danieliverant left a comment

Danieliverant Mar 7, 2023

ronnetzer Mar 11, 2023

	tap((projectName) => (this.projectName = projectName))
	distinctUntilChanged()

feat: 🎸 edit project configuration #87

Are you sure you want to change the base?

feat: 🎸 edit project configuration #87

Conversation

amanecer2levi commented Mar 2, 2023

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

nx-cloud bot commented Mar 2, 2023 • edited Loading

Nx Cloud Report

Danieliverant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danieliverant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nx-cloud bot commented Mar 2, 2023 •

edited

Loading