-
Notifications
You must be signed in to change notification settings - Fork 83
refactor: Integrate logging package #755
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
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.
Looks Great! Added a question. Also please double check all the copyright header changes too.
@@ -13,14 +13,13 @@ | |||
* See the License for the specific language governing permissions and | |||
* limitations under the License. | |||
*/ | |||
import logHelper from './modules/logging/logger'; |
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.
Why did you need to introduce the concept of logHelper here. Why importing directly from ./modules/loggin
does not work?
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.
Karma browser test was not able to locate the package unless it is defined in the default.
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! @opti-jnguyen @mikechu-optimizely it will be helpful if you guys can also take a look and provide feedback on this.
Added a couple comments @ozayr-zaviar - let me know what you think. cc: @zashraf1985 |
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.
Minus the small non-blocking notes from my comments, LGETM!
Summary
optimizely-sdk
package.Test plan
Jira
OASIS-8204