-
Notifications
You must be signed in to change notification settings - Fork 42
fix: angular tests in main #1251
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
Conversation
Summary of ChangesHello @toddbaert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a recurring test compilation failure in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
"baseUrl": "../", | ||
"paths": { | ||
"angular": [ | ||
"./dist/angular" | ||
] | ||
], | ||
"@openfeature/core": [ "../../shared/src" ], | ||
"@openfeature/web-sdk": [ "../../web/src" ] |
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.
This is the fix.
"emitDecoratorMetadata": true, | ||
"noImplicitOverride": false, | ||
"noPropertyAccessFromIndexSignature": false, | ||
"strict": false |
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 had to add these, because now the ng-test step compiles some code from the shared packages, and doesn't tolerate some stuff there without these.
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.
Code Review
This pull request aims to fix a test compilation issue by pointing the Angular tests to the src
directory of local dependencies instead of dist
. This is a good approach to ensure tests run against the latest code. However, my main concern is that several TypeScript strictness checks are disabled in tsconfig.spec.json
to make the build pass. This can hide potential bugs and should be avoided. I've left a comment to remove these overrides and fix the underlying type errors instead. I also left a comment on a type assertion that seems to be a workaround for one of these type errors.
"noImplicitOverride": false, | ||
"noPropertyAccessFromIndexSignature": false, | ||
"strict": false |
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.
Disabling these TypeScript strictness checks (noImplicitOverride
, noPropertyAccessFromIndexSignature
, strict
) can hide potential bugs and reduces the benefits of using TypeScript. It's better to address the underlying type errors directly rather than turning off the compiler checks. Please remove these overrides and fix any resulting compilation errors.
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.
This is true, but this is only for test code (tsconfig.spec.json
); without this, we'd have to make a bunch of changes to @openfeature/core
and @openfeature/web
just to satisfy it's some of it's misconceptions.
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
fc0421a
to
3d9ffe3
Compare
|
||
// only reconcile if the onContextChange method returns a promise | ||
if (typeof maybePromise?.then === 'function') { | ||
if (maybePromise && typeof maybePromise?.then === 'function') { |
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.
This is the only thing the ng-compiler was complaining about I could not ignore - obviously there's no real runtime change here, it just makes the compiler happy.
🤖 I have created a release *beep* *boop* --- ## [0.0.17](angular-sdk-v0.0.16...angular-sdk-v0.0.17) (2025-09-30) ### 🐛 Bug Fixes * angular tests in main ([#1251](#1251)) ([f42011d](f42011d)) ### Dependencies * The following workspace dependencies were updated * devDependencies * @openfeature/web-sdk bumped from ^1.5.0 to ^1.6.2 --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: Lukas Reining <lukas.reining@codecentric.de> Co-authored-by: Lukas Reining <lukas.reining@codecentric.de>
🤖 I have created a release *beep* *boop* --- ## [1.6.2](web-sdk-v1.6.1...web-sdk-v1.6.2) (2025-09-30) ### 🐛 Bug Fixes * angular tests in main ([#1251](#1251)) ([f42011d](f42011d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com>
@lukas-reining this fixes the ongoing test compilation issue with ng-test we discussed today in main.
The reason you didn't see it locally is because if we have a compiled
dist/
in../web
(the local version of@openfeature/web-sdk
) it's resolved, but the main build doesn't have this at the point the tests run.I could have fixed this by adding a compile step, but I think it's always better to run the tests against
src/
of our local peers instead ofdist/
(or else things get really weird and you can sometimes not realize you're testing against stale code)This PR fixes this issue (confirmed by deleting the
../web
dist locally, seeing the same errors, and then applying this fix).NOTE: none of these changes impact prod code - just test configuration and one TS-only code change.