-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix ReportPostPayoad to accept a label string #142
Conversation
This will allow caller to pass a label when generating an analytics report
closes #141 |
@@ -55,6 +55,9 @@ | |||
"karma-sourcemap-loader": "0.3.7", | |||
"karma-spec-reporter": "0.0.31", | |||
"karma-webpack": "3.0.0", | |||
"lodash": "^4.17.11", | |||
"mime": "^2.4.0", | |||
"parsejson": "0.0.3", |
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.
Do we use these libraries?
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 don't use them directly, but I am trying to fix the github security warning alerts. In reality, the real fix is to upgrade the packages we do use, but that requires more time I can put on it right now:
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.
oh ok! PR looks good then!
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.
could you remove these 2 lines of code? they use the old version of observable and break the new library
https://github.com/iotile/ng-iotile-cloud/blob/master/src/cloud.service.ts#L6
https://github.com/iotile/ng-iotile-cloud/blob/master/src/cloud.service.ts#L7
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.
What do you mean be remove? They are used. How do I remove them? Can I just upgrade the rs library?
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.
If you just delete those two lines of code, rs library has already been updated and ForkJoin
has already been imported on line 5 and of
is not being used.
These two lines don't break the webapp because it includes the package rxjs-compat
for backward compatibility, but that package is not included in the new library
Which can break caller libraries.
This will allow caller to pass a label when generating an analytics report.
Version 0.26.0