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

Update to Angular 19 with rework #53

Open
wants to merge 59 commits into
base: main
Choose a base branch
from

Conversation

joematthews
Copy link
Owner

@joematthews joematthews commented Jan 15, 2025

update to angular 19 with rework

Broad Stokes:

  • Use the src/ files from a fresh angular 19 project
  • Remove angular material, i18n, & ssr schematics from the project
    • Mentioned these in a new 'Opt-in Angular Schematics' section in the README
  • Add 'What It Is' and 'What It Is Not' sections to the top of the README to clarify scope
  • Convert eslint configuration to the new 'flat config' syntax.
  • Add new scripts to package.json to help with checking typescript errors in the app and tests without having to build the app or run the tests
  • Add new scrips, npm run lint:all to simplify checking all files for issues that may prevent a commit, and npm run ci:all which also checks everything and then runs unit tests that might prevent a push
  • Add new script npm run shove that will skip checks and tests and push immediately in case of emergency (fire, flood etc.)
  • Add GitHub Action workflow that runs npm run ci:all on pull request
  • Add more tips including how to set up JetBrains IDEs

new README.md

@joematthews joematthews force-pushed the update-to-angular-19-with-rework branch 4 times, most recently from a298769 to 2a0aaaa Compare January 15, 2025 23:55
@joematthews joematthews marked this pull request as ready for review January 16, 2025 00:10
@joematthews
Copy link
Owner Author

joematthews commented Jan 16, 2025

@replete I based this PR branch off of your PR branch, although I'm not too sure how many of your lines are left except the # Extreme Angular 19 in the README.md

Can you look things over? Your feedback is very much appreciated. Thank you. 🙏

@joematthews joematthews linked an issue Jan 16, 2025 that may be closed by this pull request
@joematthews joematthews marked this pull request as draft January 16, 2025 04:25
@joematthews
Copy link
Owner Author

joematthews commented Jan 16, 2025

Marking this as draft. Going to continue to refine the README package.json scripts.

replete and others added 2 commits January 15, 2025 23:54
  - remove angular material and angular ssr from base project configuration
  - convert eslint config to eslint 9
  - add mission to README.md and make further refinements
  - refine scripts in package.json
  - add github action pull request workflow
@joematthews joematthews force-pushed the update-to-angular-19-with-rework branch from 520bdbe to 71439d5 Compare January 16, 2025 05:58
@joematthews
Copy link
Owner Author

joematthews commented Jan 17, 2025

Pity they didn't adopt Playwright

Thanks for bringing this up. Check out https://angular.dev/tools/cli/end-to-end. Running ng e2e now brings up a wizard to choose from multiple options. I think I'll include a reference to this in the Optional Angular Schematics section after I test it out.

- added example eslint config for playwright to e2e section
- improved wording of intro
@joematthews
Copy link
Owner Author

@replete

I added a new e2e section in the README.md and added a example eslint config for playwright in the section. I removed references to catppuccin because it's becoming more apparent its irrelevant 😞.

@joematthews joematthews changed the title Update to angular 19 with rework Update to Angular 19 with rework Jan 18, 2025
@joematthews
Copy link
Owner Author

Alright, I'm done 'tweaking' for now. @replete Please let me know if you see anything amiss! I'm waiting on some more feedback from a few folks, and then I think I'm going to squash this monstrosity of a branch down to one commit on top of your PR branch and release on Sunday morning.

@replete
Copy link

replete commented Jan 18, 2025

Hi @joematthews,

This looks really good to me in terms of the tooling and standards laid out. That's the important thing to get right on a new project, to make sure you're doing things right from the start.

I have a couple of suggestions and will push a branch soon to see if you like it

@replete
Copy link

replete commented Jan 18, 2025

Hi @joematthews
I made some changes in this new branch in my fork, it's based on this current PR so should replay if you want to bring it into this PR:
https://github.com/replete/extreme-angular/tree/use-more-esm-in-tooling

The main change is to adopt ESM and drop CommonJS as primary standard in the tooling. I think we're at a place now where we'd all be happier with just one flavour of modules in the JS ecosystem, and its mostly there.

Changes in branch:

  • set "type": "module" in package.json - this means that the .js extension will be assumed ESM. If we really need commonJS, we can use .cjs. .mjs is only required in projects where type:module is not set in package.json. I think this is best practice nowadays as commonjs becomes legacy.
  • convert and rename eslint.config.js to ESM:
    • The rename to .js is parity with type:module being added
    • I've also renamed some of the imported objects for more precision on what they are - this kind of thing really helps when projects grow. I would have pulled { configs as esConfigs } for the eslint/js, but its apparantly a commonJS module, which is really weird, so we can't do that without eslint falling apart. A compromise
  • add .js single quote rule to .editorconfig and reformatted eslint config file for consistency
  • adopted globals.node etc instead of { node } for the globals packages - node prettier aren't great names in terms of potential conflicts, and don't represent what they actually are. We could have done `{ node as nodeGlobal, ... } etc, but would be unnecessarily terse, and globals.whatever is already very clear what it is. This is a principle I've found really important in shared codebases - name the thing very closely to what it is

See what you think, but this is something I'd want in any modern project, make everything ESM and use .cjs if I really have to. Most popular toolings now support ESM, and that's the standard now so why not.

The other thing that bugs me that as a user of this package, is the 2-space indent on JS/TS source, I personally find it less readable, but of course its subjective, but it doesn't seem as good as 4 to me generally. Again, not a big deal for an end user would just change it and fix all, and if its good enough for airbnb. Well, it is an interesting point though, I see the benefit of more code being visible, but it just seems for most people harder to read? I wonder if there are any surveys on this... either way genuinely not a big deal for me. EDIT: I'm going to build my next project in 2sp and see if I get used to it

@joematthews
Copy link
Owner Author

I couldn't agree more. I only stuck with CommonJS for eslint.ocnfig.js because that's what angular-eslint produced -- but as I got more and more annoyed when incorporating libraries in to the config.

Switching to ECMAScript modules will also make it possible for prettier-plugin-organize-imports to do it's magic in config files too.

I'll checkout your branch very soon.

@replete
Copy link

replete commented Jan 18, 2025

I did notice the automatic import organizing after I ported eslint config to ESM. Very nice!

If these changes make it in will need an update in README.md for the playwright code sample, have to pop out for the rest of the day but will have a look what's happening tomorrow.

@joematthews
Copy link
Owner Author

joematthews commented Jan 18, 2025

The other thing that bugs me that as a user of this package, is the 2-space indent on JS/TS source.

I get it. Hot topic. I don't disagree about the readability. I am used to it at this point.

I think it's best to keep 2 character indentation for these reasons:

Aside from all that, for me personally, I really like the ability to do side-by-side editors and in my experience that really sucks in JS/TS unless indentation 2 and the line width is 80.

All that said, I have have become quite used to Prettier's output for files, but I do remember saying 'ewww' out loud the first time I formatted all files in a project hah.

Changing the indentation and line width is pretty easy. In the .editorconfig set the desired indent_size in the overrides for the desired file types:

[*.ts]
quote_type = single
indent_size = 4
ij_typescript_use_double_quotes = false

And then in the .prettierrc.json file set the tabWidth & printWidth to your desired values:

{
    "tabWidth": 4,
    "printWidth": 120,
    "htmlWhitespaceSensitivity": "ignore",
    "plugins": [
        "prettier-plugin-sh",
        "prettier-plugin-css-order",
        "prettier-plugin-organize-imports"
    ]
}

And run npm run format, then review, and commit the results.

Should I include these instructions in the tips and tricks? 🤔.

@replete
Copy link

replete commented Jan 18, 2025

Hi Joe, I probably didn't need to think out loud there, I did notice the angular docs after writing the comment. I will adapt and I suspect end up liking it...

@replete
Copy link

replete commented Jan 18, 2025

What would be really extreme, would be forcing tabs, which would let anyone display their code however they want 🤣

After contracting for some years, I learned that tabs are not a hill worth dying on...

@joematthews
Copy link
Owner Author

joematthews commented Jan 18, 2025

Hi Joe, I probably didn't need to think out loud there, I did notice the angular docs after writing the comment. I will adapt and I suspect end up liking it...

No, it was important to bring up! It has come up in every team I've been on when setting up Prettier. Some teams have opted for longer line widths and using double quotes in TS files. But, I will admit, I've talked them all out of 4 spaces! haha

Not using tabs was historically a missed opportunity. But there can be conflicts between editors that expect spaces. The .editorconfig file can help a lot, basically instructing the editor's default behavior (if they have a plugin for it).

If you want to try out tabs, you can look at the useTabs option for Prettier. This will allow the pre-commit to reformat changes correctly with tabs before commit (no matter what other devs are doing in their editor).

That's the beauty of prettier --write in the pre-commit. Doesn't help if they do commit --no-verify though 😞 That's where formatting checks in pre-push and in CI can help.

- specify `src/**/*.spec.ts` files to prevent conflict with e2e
- update playwright eslint example to mjs import
@joematthews
Copy link
Owner Author

@replete I merged in your changes. Thank you very much!

The only fix I did was reverting the files proprety in the jasmine section to 'src/**/*.spec.ts' so they do not conflict with potential 'e2e/**/*.spec.ts' files if they opt for playwright. (this was something I corrected for earlier in my barrage of pushes lol)

@replete
Copy link

replete commented Jan 18, 2025

Awesome, it's looking tidy! I'll test the new setup again when it all lands in main with a new project next week.

I'm churning through trying to figure out best practices for all the 'new' Angular stuff – all the books I'm reading on this are still mostly for Angular 17 and it seems to me like zoneless + signals are a direction I should be headed, but there'll be some lag I guess with best practices so it feels like early days with all that.

@joematthews
Copy link
Owner Author

joematthews commented Jan 19, 2025

@replete yeah, I'm pretty new to both zoneless and signals myself.

I have a few projects coming up and I think I will keep zonejs (for now) and use signals more (or exclusively if possible).

In the zoneless docs it mentions using OnPush to be safe. In the new eslint.config.js I'll probably be setting this to 'error' in my projects, so it enforces onpush for every component:

      '@angular-eslint/prefer-on-push-component-change-detection': 'off',

I don't want to make this 'warn' or 'error' by default quite yet in extreme-angular (because it's annoying if you're not using onpush at all, which most peoople are not I think), But it's a hella cool rule to have available.

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.

Update to Angular 18 / 19 Github Action to lint, check formatting & test
2 participants