-
-
Notifications
You must be signed in to change notification settings - Fork 207
feat(custom-esbuild): add unit-test builder
#1935
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
base: master
Are you sure you want to change the base?
Conversation
02e8d60 to
67ab831
Compare
|
Thank you for your contribution! In general LGTM, a few things:
|
|
Thanks a lot of the review!
|
…tible versions to be used during tests
9966393 to
f997964
Compare
| "resolutions": { | ||
| "@cypress/schematic/@angular-devkit/architect": ">=0.2000.0 < 0.2100.0", | ||
| "@cypress/schematic/@angular-devkit/core": "^20.0.0", | ||
| "@cypress/schematic/@angular-devkit/schematics": "^20.0.0", | ||
| "@cypress/schematic/@schematics/angular": "^20.0.0" | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't get the reason for these resolutions - looks like everything works fine without them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly historical, if it works now without then all good.
|
@just-jeb added tests and fixed the build. Could you take a look once more please? |
examples/custom-esbuild/sanity-esbuild-app-esm/src/app/app.component.spec.ts
Show resolved
Hide resolved
| "@angular/cli": "20.0.3", | ||
| "@angular/compiler-cli": "20.0.3", | ||
| "@angular/language-service": "20.0.3", | ||
| "@angular-devkit/build-angular": "^20.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to use ^ versions of angular packages here as otherwise yarn installs different versions of those packages for examples and packages which results in a broken build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably just bump the version here to match the minimal required package versions (which I though you updated). In general the examples are designed to serve as a real app with strict dependencies, partially in order to validate this issue - version matching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the minimal versions of the packages only in custom-esbuild package as this is the only one that requires a version bump. There is no need to bump versions in other packages as they're unrelated.
But the real reason I added ^ in all examples is because without it yarn installs different versions of angular modules for @angular-builders/custom-esbuild package and for examples/custom-esbuild/* examples.
For instance, it installs @angular/[email protected] for the @angular-builders/custom-esbuild and @angular/[email protected] for the examples/custom-esbuild/sanity-esbuild-app:
The similar issue happens for custom-webpack examples - yarn installs different versions of @angular-devkit/build-angular for the @angular-builders/custom-webpack package and for e.g. examples/custom-webpack/sanity-app which leads to a different versions of webpack being installed:
This webpack versions mismatch causes the following error during yarn ci:
TypeError: The 'compilation' argument must be an instance of Compilation
So the only solution I found to make yarn install a single version of each angular package (dedupe them) is to loosen package versions in examples.
@just-jeb maybe you have some other thoughts?
| "prettier": "^3.0.0", | ||
| "ts-jest": "29.2.5" | ||
| "ts-jest": "29.2.5", | ||
| "vitest": "3.2.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't add vitest to example's package.json as in this case angular unit-test builder can't find it and ng test fails with the corresponding error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vitest should be a peer dependency in the custom-esbuild package, and a dev dependency in the example app. But having it here is kinda workaroundish.
|
@just-jeb could you take a look one more time? |
|
Sorry guys, I was off the radar for quite a while, looking now 👀. |
@angular-builders/custom-esbuild:unit-test builderunit-test builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@th0r Very nice job, love it!
Left a few minor comments, mostly about versioning and deps, mind taking a look?
I'd say it's already mergeable but if we addressed those first would be awesome.
| "@angular/cli": "20.0.3", | ||
| "@angular/compiler-cli": "20.0.3", | ||
| "@angular/language-service": "20.0.3", | ||
| "@angular-devkit/build-angular": "^20.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably just bump the version here to match the minimal required package versions (which I though you updated). In general the examples are designed to serve as a real app with strict dependencies, partially in order to validate this issue - version matching.
| "@angular/animations": "^20.0.0", | ||
| "@angular/common": "^20.0.0", | ||
| "@angular/compiler": "^20.0.0", | ||
| "@angular/core": "^20.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here regarding the versions
| "@angular/common": "^20.0.0", | ||
| "@angular/compiler": "^20.0.0", | ||
| "@angular/core": "^20.0.0", | ||
| "@angular/forms": "^20.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here regarding the versions
| "@angular/common": "^20.0.0", | ||
| "@angular/compiler": "^20.0.0", | ||
| "@angular/core": "^20.0.0", | ||
| "@angular/forms": "^20.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here regarding the versions
| "peerDependencies": { | ||
| "@angular/common": "20.0.3", | ||
| "@angular/core": "20.0.3" | ||
| "@angular/common": "^20.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here regarding the versions
| "@angular/animations": "^20.0.0", | ||
| "@angular/common": "^20.0.0", | ||
| "@angular/compiler": "^20.0.0", | ||
| "@angular/core": "^20.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here regarding the versions
| "@angular/animations": "^20.0.0", | ||
| "@angular/common": "^20.0.0", | ||
| "@angular/compiler": "^20.0.0", | ||
| "@angular/core": "^20.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here regarding the versions
| "resolutions": { | ||
| "@cypress/schematic/@angular-devkit/architect": ">=0.2000.0 < 0.2100.0", | ||
| "@cypress/schematic/@angular-devkit/core": "^20.0.0", | ||
| "@cypress/schematic/@angular-devkit/schematics": "^20.0.0", | ||
| "@cypress/schematic/@schematics/angular": "^20.0.0" | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly historical, if it works now without then all good.
| "prettier": "^3.0.0", | ||
| "ts-jest": "29.2.5" | ||
| "ts-jest": "29.2.5", | ||
| "vitest": "3.2.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vitest should be a peer dependency in the custom-esbuild package, and a dev dependency in the example app. But having it here is kinda workaroundish.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Issue Number: #1932
What is the new behavior?
This PR adds an
@angular-builders/custom-esbuild:unit-testbuilder which is based on the@angular/build:unit-testexperimental builder, but allows to specify a list of custom ESBuild plugins viapluginsoption.Does this PR introduce a breaking change?