Skip to content
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: added typing for patterns #220

Merged
merged 5 commits into from
Aug 12, 2020
Merged

Conversation

lkhari
Copy link
Contributor

@lkhari lkhari commented Jul 2, 2020

What did you implement:

I've created some typing for correlation-middleware, to add some clarity on where the correlationId and logger has been added, since the sqs capture correlationId puts the logger on the event and the capture streams exposes them on the context.

I've also exposed the Logger in correlation-middleware and appended it to the patterns. This just to reduce having to import both Logger and correlation-middleware. Due to the assumption that if I got the correlation-middleware I most likely will be using the logger.

How did you implement it:

Added a index.d.ts file and on the index.js exposed the logger as a named export.

How can we verify it:

Create a typescript project import logger and see ;)

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / projects / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: Yes
Is it a breaking change?: NO

@lkhari
Copy link
Contributor Author

lkhari commented Jul 13, 2020

Any opinions on the work done @gsingh1 ? I haven't exposed all of lambda Types, thinking it shouldn't be our responsibility to deal with all of them. So I've only exposed the types in which we've affected.

Copy link
Contributor

@gsingh1 gsingh1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @lkhari, thanks for this. LGTM and I think we can start adding the others as and when required.

Copy link
Contributor

@simontabor simontabor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lkhari and sorry for the slow reviews. Types look fine (haven't tested directly myself though)

Exposing Log should be moved to an issue/feature proposal so this PR doesn't get held up further

@lkhari lkhari changed the title feat: added typing for patterns and exposed logger feat: added typing for patterns Jul 24, 2020
Copy link
Contributor

@simontabor simontabor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the changes! Just one small comment for default Log value

@lkhari
Copy link
Contributor Author

lkhari commented Aug 12, 2020

@simontabor or @gsingh1 can this be merged? The ability for custom logger shouldn't affect this story, since this this is just types for the current library.

@gsingh1 gsingh1 merged commit 95d54f6 into getndazn:master Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants