-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(logging): add new logging category #12759
feat(logging): add new logging category #12759
Conversation
a20b949
to
f3967b7
Compare
f3967b7
to
d42e148
Compare
…ify-js into central-logger-addcategory
packages/logger/index.js
Outdated
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 the logger
package is not expected to be published as CDN bundle, please remove Webpack related settings.
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.
thanks, removed webpack related settings.
Co-authored-by: Hui Zhao <10602282+HuiSF@users.noreply.github.com>
Co-authored-by: Hui Zhao <10602282+HuiSF@users.noreply.github.com>
…to central-logger-addcategory
…to central-logger-addcategory
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.
Some comments, but overall looks good!
packages/logger/package.json
Outdated
"tslib": "^2.5.0" | ||
}, | ||
"peerDependencies": { | ||
"@aws-amplify/core": "^6.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.
Not required yet, but at some point before merge we'll need to update this to the version that will contain the new logger utils (across all packages)
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.
Ahhhh yes got it, all packages having peerDependencies
on core would need to update to use the latest core version. This to make sure they have access to latest logger that's implemented in core
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.
Removing this for now, will add it back when required.
EDIT: added back (reference)
Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>
Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>
packages/logger/package.json
Outdated
"peerDependencies": { | ||
"@aws-amplify/core": "^6.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.
IMO we should leave this in place for our pre-publish script to update it during testing and so that we don't forget to update when we bump the other packages
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.
makes sense, added back
packages/logger/tsconfig.build.json
Outdated
@@ -0,0 +1,5 @@ | |||
{ |
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.
Is there any reason this package still needs a separate build configuration, or can it just share the same tsconfig.json
like most of our packages do 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.
I think this will be unified in the eslint migration, so we can keep it. wdyt
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.
It depends on the timing of that vs this I think? If this feature branch is merged after that migration then it will be missed by the migration right?
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.
good point, adding a todo to make sure this is tracked
packages/logger/__tests__/providers/cloudwatch/apis/getCloudWatchProvider.test.ts
Outdated
Show resolved
Hide resolved
5a69717
to
ed38a6e
Compare
ed38a6e
to
dc7b7f2
Compare
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
// TODO: pending implementation | ||
export const createLogger = (): void => {}; |
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.
nit/suggestion: could define the actual return type here, and leave the implementation throw new Error('API has not been implemented.')
, this would be more continuous.
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.
that's a good point. We'll be wiring up all these PRs once the other Providers PR gets merged.
So we should shortly be replacing all these. If anything's pending will add the throw new Error('API has not been implemented.')
wdyt
Description of changes
logging
categoryenable
disable
flushLogs
createLogger
cloudwatch
APIsenable
disable
getProvider
console
APIsenable
disable
getProvider
Description of how you validated changes
verified apis are visible
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.