-
Notifications
You must be signed in to change notification settings - Fork 28
Two fixes for our spa's integration with the GoodData library. #298
Two fixes for our spa's integration with the GoodData library. #298
Conversation
Codecov Report
@@ Coverage Diff @@
## master #298 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 60 60
Lines 1440 1440
Branches 227 227
=====================================
Hits 1440 1440
Continue to review full report at Codecov.
|
@@ -84,7 +84,7 @@ | |||
"raw-loader": "0.5.1", | |||
"reflect-metadata": "0.1.10", | |||
"remap-istanbul": "0.9.5", | |||
"rxjs": "5.4.2", |
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.
Hey @Blackbaud-MikitaYankouski, in your testing, did you run into any problems upgrading to this version of rxjs. Our dependency chain isn't crystal clear, and we've ran into problems when upgrading the dependency in builder, but not upgrading it in the skyux2 components.
https://github.com/blackbaud/skyux2/blob/master/package.json#L108
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.
Just re-ran the all the tests and haven't ran into any issues!
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.
The template also works with these changes. A point of note: UX does not directly depend on rxjs, so the mixmatched versions shouldn't affect a SPA.
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.
We are using a third party library that is required by our app that is more heavily dependent on rxjs, and they use this higher version and having two version in our node_modules gave us build errors.
@@ -30,6 +30,8 @@ require('zone.js/dist/sync-test'); | |||
require('zone.js/dist/proxy'); | |||
require('zone.js/dist/jasmine-patch'); | |||
|
|||
require('reflect-metadata'); |
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.
@Blackbaud-MikitaYankouski Mind providing more information on how this affects your unit tests?
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.
Without this line the unit tests do not run and the code coverage results aren't printed so our build ends up failing as well.
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.
LGTM. Nothing was affected during my local tests.
package.json
Outdated
@@ -112,4 +112,4 @@ | |||
"proxyquire": "1.8.0", | |||
"rimraf": "2.6.1" | |||
} | |||
} | |||
} |
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 noticed your editor removed the trailing newline character from both these files. Those newlines need to be preserved for TSLint and so would cause problems if you had edited a .ts file. The project's .editorconfig file specifies that a newline should be added to a file on save, which makes me think your editor is not configured to recognize the .editorconfig file. If you're using VSCode, you can install the EditorConfig plugin to fix this.
Rxjs library update to match the version of GoodData and adding a require to reflect-metadata so our unit tests are able to run and produce code coverage.