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(angular): add Angular Renaissance & update Angular snippets #245

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

LcsGa
Copy link

@LcsGa LcsGa commented Jul 20, 2024

Solves the issue #241

@LcsGa LcsGa closed this Jul 20, 2024
@LcsGa LcsGa reopened this Jul 20, 2024
@LcsGa LcsGa marked this pull request as draft July 20, 2024 22:12
@LcsGa LcsGa force-pushed the feature/angular-renaissance branch 3 times, most recently from 9d57e98 to e4f2c7e Compare July 22, 2024 07:08
@matschik
Copy link
Owner

The build is failing, can you fix it ? As I said it's surely because of a change in frameworks.mjs

@LcsGa
Copy link
Author

LcsGa commented Jul 24, 2024

I can take another look at it tomorrow but I couldn't find what was the issue there

@LcsGa
Copy link
Author

LcsGa commented Jul 24, 2024

Well, I took a look at the code of the .mjs file but I really don't see what could break there... I'm not really familiar with this kind of config files too so I may be missing something but I can't figure out what 😅

template: `<p>Page title: {{ pageTitle }}</p>`,
})
export class PagetitleComponent implements OnInit {
pageTitle = "";
Copy link

Choose a reason for hiding this comment

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

Any reason not to use signal here?

Copy link
Author

Choose a reason for hiding this comment

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

Not at all, probably just a fail from my massive copy pasting

<p>I am {{ isAvailable() ? "available" : "not available" }}</p>
`,
})
export class UserprofileComponent {
Copy link

Choose a reason for hiding this comment

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

p in Profile should be uppercase :) UserprofileComponent

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you but my goal was to make the least changes as the current version with is also lowercase.
I guess it's not a big deal and I could indeed update it both here and in the older (current) version

Copy link
Owner

Choose a reason for hiding this comment

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

The primary goal is to show Angular best practices, so if it musts be uppercase in the two cases, let's change both naming.

@ogix
Copy link

ogix commented Oct 23, 2024

Can anyone tell what status of this PR is? Are there any issues?

@LcsGa LcsGa marked this pull request as ready for review October 24, 2024 10:52
@LcsGa
Copy link
Author

LcsGa commented Oct 24, 2024

The status of this PR remains unchanged. I still have the same issues that the ones above and didn't have the time (and probably the knowledge) to fix them.

If you have any ideas to resolve them, I'd be glad to try them

@ogix
Copy link

ogix commented Oct 24, 2024

Ok, thanks. Maybe I will have a look if I find time for this.

BTW, Angular 19 is making components "standalone: true" by default, so we can remove later those flags.

@matschik
Copy link
Owner

@LcsGa You introduce a bug and I have to solve it ? That's a little unfair.
To make it fair, can you at least give me some steps to reproduce and error logs ?
I'll try to fix it afterwards.

@LcsGa
Copy link
Author

LcsGa commented Oct 24, 2024

@LcsGa You introduce a bug and I have to solve it ? That's a little unfair. To make it fair, can you at least give me some steps to reproduce and error logs ? I'll try to fix it afterwards.

I never told you that you had to solve it yourself.

I'm not familiar with the way of doing in the framework.mjs file...
I first simply tried to copy/paste + update the angular config into angular-renaissance.
Then I did what was written in the readme with the problem you told me it probably was but none of it worked and I'm running out of ideas...

I don't clearly remember how to reproduce it righ now + I changed my computer but I will soon clone it once more, retry, and give more informations if :

  • I still cand find any fix myself
  • I have more to provide

The most that I remember at the moment is that I simply tried to build the projects with the provided commands but it failed right away

@matschik
Copy link
Owner

matschik commented Oct 24, 2024

Can you give the error logs to begin the investigation ?

@LcsGa
Copy link
Author

LcsGa commented Oct 25, 2024

I didn't have the opportunity to retry yet but the error I faced was:

pnpm run dev

> component-party.dev@2.0.0 dev /home/lucas/dev/component-party.dev
> vite

[generateFrameworkContent] Generating framework content files...
TypeError: Cannot read properties of undefined (reading 'push')
    at file:///home/lucas/dev/component-party.dev/vite.config.js.timestamp-1721509708644-d2f6b8691061.mjs:2843:40
    at async Promise.all (index 4)
    at async generateContent (file:///home/lucas/dev/component-party.dev/vite.config.js.timestamp-1721509708644-d2f6b8691061.mjs:2831:7)
    at async build (file:///home/lucas/dev/component-party.dev/vite.config.js.timestamp-1721509708644-d2f6b8691061.mjs:2981:7)
    at async PluginContext.buildStart (file:///home/lucas/dev/component-party.dev/vite.config.js.timestamp-1721509708644-d2f6b8691061.mjs:2997:9)
    at async Promise.all (index 6)
    at async PluginContainer.hookParallel (file:///home/lucas/dev/component-party.dev/node_modules/.pnpm/vite@5.3.4_@types+node@20.14.11_terser@5.31.3/node_modules/vite/dist/node/chunks/dep-D8YhmIY-.js:49481:5)
    at async PluginContainer.buildStart (file:///home/lucas/dev/component-party.dev/node_modules/.pnpm/vite@5.3.4_@types+node@20.14.11_terser@5.31.3/node_modules/vite/dist/node/chunks/dep-D8YhmIY-.js:49484:5)
    at async file:///home/lucas/dev/component-party.dev/node_modules/.pnpm/vite@5.3.4_@types+node@20.14.11_terser@5.31.3/node_modules/vite/dist/node/chunks/dep-D8YhmIY-.js:63425:7
    at async httpServer.listen (file:///home/lucas/dev/component-party.dev/node_modules/.pnpm/vite@5.3.4_@types+node@20.14.11_terser@5.31.3/node_modules/vite/dist/node/chunks/dep-D8YhmIY-.js:63440:9)
error when starting dev server:
TypeError: Cannot read properties of undefined (reading 'push')
    at file:///home/lucas/dev/component-party.dev/vite.config.js.timestamp-1721509708644-d2f6b8691061.mjs:2843:40
    at async Promise.all (index 4)
    at async generateContent (file:///home/lucas/dev/component-party.dev/vite.config.js.timestamp-1721509708644-d2f6b8691061.mjs:2831:7)
    at async build (file:///home/lucas/dev/component-party.dev/vite.config.js.timestamp-1721509708644-d2f6b8691061.mjs:2981:7)
    at async PluginContext.buildStart (file:///home/lucas/dev/component-party.dev/vite.config.js.timestamp-1721509708644-d2f6b8691061.mjs:2997:9)
    at async Promise.all (index 6)
    at async PluginContainer.hookParallel (file:///home/lucas/dev/component-party.dev/node_modules/.pnpm/vite@5.3.4_@types+node@20.14.11_terser@5.31.3/node_modules/vite/dist/node/chunks/dep-D8YhmIY-.js:49481:5)
    at async PluginContainer.buildStart (file:///home/lucas/dev/component-party.dev/node_modules/.pnpm/vite@5.3.4_@types+node@20.14.11_terser@5.31.3/node_modules/vite/dist/node/chunks/dep-D8YhmIY-.js:49484:5)
    at async file:///home/lucas/dev/component-party.dev/node_modules/.pnpm/vite@5.3.4_@types+node@20.14.11_terser@5.31.3/node_modules/vite/dist/node/chunks/dep-D8YhmIY-.js:63425:7
    at async httpServer.listen (file:///home/lucas/dev/component-party.dev/node_modules/.pnpm/vite@5.3.4_@types+node@20.14.11_terser@5.31.3/node_modules/vite/dist/node/chunks/dep-D8YhmIY-.js:63440:9)
 ELIFECYCLE  Command failed with exit code 1.

@matschik
Copy link
Owner

matschik commented Oct 25, 2024

@LcsGa I found the problem. In frameworks.mjs, you declared Angular Renaissance framework with id "angularRenaissance". But in content directory, you used "angular-renaissance" instead of the id "angularRenaissance".

See "emberOctane" for example

@matschik matschik self-assigned this Oct 25, 2024
@matschik
Copy link
Owner

matschik commented Oct 25, 2024

@LcsGa Why did you declare in Angular examples "AppModule" with "@NgModule" and not in Angular Renaissance ? I think it's not necessary since it's just to learn about the syntax.

Let's make it @LcsGa ! Your branch is working now :)
Can you apply previously suggested modifications by @ogix ?

declarations: [NameComponent],
exports: [NameComponent],
})
export class NameModule {}
Copy link

Choose a reason for hiding this comment

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

Just use standalone: true in all components and get rid of the @NgModule. They are no longer needed.
In Angular v19 standalone: true will be the default.

export class InputfocusedComponent implements OnInit {
inputRef = viewChild.required<ElementRef<HTMLInputElement>>("inputRef");

ngOnInit() {
Copy link

Choose a reason for hiding this comment

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

Might be a good idea to replace with an effect().

export class TimeComponent implements OnDestroy {
time = signal(new Date().toLocaleTimeString());

timer = setInterval(
Copy link

Choose a reason for hiding this comment

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

Just use rxjs + toSignal() and get rid of ngOnDestroy:

time$ = timer(0, 1e3)
    .pipe(
        map(() => new Date().toLocaleTimeString())
    );
time = toSignal(this.time$);

};

updateUsername(username: string) {
this.user.username = username;
Copy link

Choose a reason for hiding this comment

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

Immutability 😉

imports: [FormsModule],
selector: "app-input-hello",
template: `
<p>{{ text }}</p>
Copy link

Choose a reason for hiding this comment

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

Should be {{ text() }} with parenthesis since it's signal.

imports: [FormsModule],
selector: "app-pick-pill",
template: `
<div>Picked: {{ picked }}</div>
Copy link

Choose a reason for hiding this comment

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

picked()

import { AppModule } from "./app.module";

platformBrowserDynamic()
.bootstrapModule(AppModule)
Copy link

Choose a reason for hiding this comment

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

No more modules.

bootstrapApplication(AppComponent, appConfig)
    .catch((err) => console.error(err));

@matschik matschik marked this pull request as draft October 25, 2024 17:57
@matschik matschik changed the title feat(angular): update the existing files + add the new angular (renaissance) practices feat(angular): add Angular Renaissance & update Angular snippets Oct 25, 2024
@LcsGa
Copy link
Author

LcsGa commented Oct 25, 2024

@LcsGa I found the problem. In frameworks.mjs, you declared Angular Renaissance framework with id "angularRenaissance". But in content directory, you used "angular-renaissance" instead of the id "angularRenaissance".

See "emberOctane" for example

Oh my... I really didn't notice that... The most time consuming bugs are always the most stupid ones I guess...

@LcsGa
Copy link
Author

LcsGa commented Oct 25, 2024

@LcsGa Why did you declare in Angular examples "AppModule" with "@NgModule" and not in Angular Renaissance ? I think it's not necessary since it's just to learn about the syntax.

Let's make it @LcsGa ! Your branch is working now :)
Can you apply previously suggested modifications by @ogix ?

I added the modules on purpose! Before angular v14 that was mandatory to create components.
If I wanted to highlight how it improved and truly compare the two of them, I had to add them in v13-
In angular renaissance, you simply have the standalone property in the @Component and it will even be true by default in one month!

Yeah sure! I can do that tomorrow morning I think! 😁

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.

4 participants