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

MI-72: Update template to match with recent changes #1090

Merged
merged 30 commits into from
Nov 28, 2024

Conversation

kai-nguyen-aligent
Copy link
Contributor

@kai-nguyen-aligent kai-nguyen-aligent commented Oct 28, 2024

This PR aims to keep our template up to date with the following changes:

  1. Adopt new .editorconfig from @aligent/ts-code-standards package
  2. Update devDependencies & added maintenance notes
  • remove tslib
  • replace deprecated @nx/lint with @nx/eslint
  • few other package version fix up to ensure the template works correctly.
  1. Update template to use vitest v1.0.4 (match with nx v17.3.0 requirement)
  2. Clean up tsconfig.base.json and other tsconfig.json.
  3. Update serverless.yml.template to pack lambda individually. This reduce lambda package size.

@tvhees
Copy link
Collaborator

tvhees commented Oct 28, 2024

@kai-nguyen-aligent this looks great, thanks for covering so many things.

Should we be importing/extending from @aligent/ts-code-standards for tsconfig now? I don't know if importing is possible for .editorconfig files

@kai-nguyen-aligent
Copy link
Contributor Author

@tvhees I have successfully update and config this to match with @aligent/ts-code-standards. It works well now!
However, this merge is now blocked because we need to configure github action so it's able to access Aligent private NPM registry.

@tvhees
Copy link
Collaborator

tvhees commented Oct 30, 2024

@tvhees I have successfully update and config this to match with @aligent/ts-code-standards. It works well now! However, this merge is now blocked because we need to configure github action so it's able to access Aligent private NPM registry.

@TheOrangePuff @AdamJHall can we get this looked at, I assume it's a devops task?

@kai-nguyen-aligent
Copy link
Contributor Author

@tvhees I have successfully update and config this to match with @aligent/ts-code-standards. It works well now! However, this merge is now blocked because we need to configure github action so it's able to access Aligent private NPM registry.

@TheOrangePuff @AdamJHall can we get this looked at, I assume it's a devops task?

@TheOrangePuff @AdamJHall I have added the workflow configuration but not will need your help to add the secret & double check my code at this commit

@AdamJHall
Copy link
Contributor

Hey @kai-nguyen-aligent,

Just to confirm which registry should we be authing against? Is it our internal private registry? If so should we instead be making that package public?

@kai-nguyen-aligent
Copy link
Contributor Author

Hey @kai-nguyen-aligent,

Just to confirm which registry should we be authing against? Is it our internal private registry? If so should we instead be making that package public?

@AdamJHall Yes, we're authing against our internal private registry to install @aligent/ts-code-standards. Regarding making it public, how do you think @tvhees? I don't see any reason for not making it public myself.

@AdamJHall
Copy link
Contributor

Hey @kai-nguyen-aligent,
Just to confirm which registry should we be authing against? Is it our internal private registry? If so should we instead be making that package public?

@AdamJHall Yes, we're authing against our internal private registry to install @aligent/ts-code-standards. Regarding making it public, how do you think @tvhees? I don't see any reason for not making it public myself.

No worries, in the meantime then we just need to update your workflow file from

"//registry.npmjs.org/:_authToken=${{ secrets.ALIGENT_NPM_TOKEN }}" > ~/.npmrc

to

"//npm.corp.aligent.consulting/:_authToken=${{ secrets.ALIGENT_NPM_TOKEN }}" > ~/.npmrc

@kai-nguyen-aligent
Copy link
Contributor Author

@tvhees This is now ready for another review. I have migrated our @aligent/ts-code-standard in to this template. A notable change is that now we run eslint directly instead of @nx/eslint plugin. This is because @nx/eslint does not support eslint configuration in ESM format yet. This change can be found in tools/serverless-plugin/project.json and tools/serverless-plugin/src/generators/service/generator.ts.

@tvhees
Copy link
Collaborator

tvhees commented Oct 31, 2024

@AdamJHall @kai-nguyen-aligent yeah I agree our code standards could be open source. Not sure why I didn't think of that before.

@tvhees
Copy link
Collaborator

tvhees commented Oct 31, 2024

@kai-nguyen-aligent having checked the requirements around the private registry, I don't think we should merge this until the code standards are open source. It doesn't feel right to me to have an 'open source' template that requires logging in to our registry, refers to private bitbucket for code standards etc.

That's a learning for the future too I suppose.

@kai-nguyen-aligent
Copy link
Contributor Author

@AdamJHall When you have time, could you please help me to remove the NPM token for our internal registry? @aligent/ts-code-standard is now published and I already fixed the workflow.

@AdamJHall
Copy link
Contributor

@AdamJHall When you have time, could you please help me to remove the NPM token for our internal registry? @aligent/ts-code-standard is now published and I already fixed the workflow.

Done :)

@kai-nguyen-aligent
Copy link
Contributor Author

kai-nguyen-aligent commented Nov 8, 2024

@tvhees Please hold off this PR until we merge the one in ts-code-standard to support CJS (aligent/ts-code-standards#13)
cc: @TheOrangePuff

@tvhees
Copy link
Collaborator

tvhees commented Nov 20, 2024

@kai-nguyen-aligent the linked PR is now merged. Do we need to make any further changes here?

@kai-nguyen-aligent
Copy link
Contributor Author

@tvhees I'vejust update the node version to the latest supported by Lamda (60875e3). Nothing else need to be done and ready for review.

Copy link

@ryanrixxh ryanrixxh left a comment

Choose a reason for hiding this comment

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

LGTM.

One thing worth bringing up though is the Nx version. Probably justifies its own ticket, but the migration from 17 to 19 went pretty smooth in my fork - figure we can it in core no issues, since we are updating stuff anyway.

@kai-nguyen-aligent
Copy link
Contributor Author

LGTM.

One thing worth bringing up though is the Nx version. Probably justifies its own ticket, but the migration from 17 to 19 went pretty smooth in my fork - figure we can it in core no issues, since we are updating stuff anyway.

Yes, Ryan. Upgrading to latest Nx version should be easy. The other part is migration to yarn 🧶

Copy link
Collaborator

@tvhees tvhees left a comment

Choose a reason for hiding this comment

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

Happy to go ahead with merging this.

@tvhees
Copy link
Collaborator

tvhees commented Nov 28, 2024

@kai-nguyen-aligent does this PR change the standard developer experience at all, or should it be mostly the same in terms of setting up a new service?

@tvhees tvhees merged commit 86df855 into main Nov 28, 2024
1 check passed
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