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

Convert createWriteStream to createWriteStreamSync #57

Closed

Conversation

devinrhode2
Copy link

@devinrhode2 devinrhode2 commented Mar 4, 2022

Users will get an import error when updating.
I think it should be pretty easy to fix.

This is a smaller diff of just the src changes, with no refactoring, no test updates, no doc updates, etc: #59
It will be easier first to understand the src changes in that PR

Checklist

  • run npm run test
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code Of Conduct

Also adding a jsdoc typescript signature.

Closes #56

@devinrhode2 devinrhode2 force-pushed the breaking-createWriteStreamSync branch 2 times, most recently from 17e20d5 to 979c138 Compare March 5, 2022 10:08
@devinrhode2
Copy link
Author

Running tests locally, I'm getting some confusing timeouts..

> pino-applicationinsights@2.1.0 test /Users/devinrhode2/repos/Atlas/pino-applicationinsights
> standard && tap test/*.test.js --coverage --100

 PASS  test/streams.test.js 4 OK 31.784ms
test/index.test.js 2> ApplicationInsights:An invalid instrumentation key was provided. There may be resulting telemetry loss [ 'blablabla' ]
test/applicationinsights.test.js 2> ApplicationInsights:An invalid instrumentation key was provided. There may be resulting telemetry loss [ 'blablabla' ]
 FAIL  test/applicationinsights.test.js
 ✖ timeout!

  test: TAP
  signal: SIGTERM
  handles:
    - type: TLSSocket
      events:
        - close
        - end
        - newListener
        - secure
        - session
        - free
        - timeout
        - agentRemove
        - error
  expired: TAP
  stack: |
    emit (node_modules/signal-exit/index.js:78:11)
    process.listener (node_modules/signal-exit/index.js:92:7)

 FAIL  test/index.test.js
 ✖ timeout!

  test: TAP
  signal: SIGTERM
  handles:
    - type: TLSSocket
      events:
        - close
        - end
        - newListener
        - secure
        - session
        - free
        - timeout
        - agentRemove
        - error
  expired: TAP
  stack: |
    emit (node_modules/signal-exit/index.js:78:11)
    process.listener (node_modules/signal-exit/index.js:92:7)
    
....

------------------------|---------|----------|---------|---------|-------------------
File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
------------------------|---------|----------|---------|---------|-------------------
All files               |   38.75 |    20.83 |    37.5 |   45.45 |                   
 applicationinsights.js |   16.95 |        5 |   16.67 |   21.74 | 9,18-76,83-87     
 index.js               |     100 |      100 |     100 |     100 |                   
 streams.js             |     100 |      100 |     100 |     100 |                   
------------------------|---------|----------|---------|---------|-------------------

@devinrhode2
Copy link
Author

devinrhode2 commented Mar 6, 2022

Turns out, test output is unchanged relative to master, diff here: e283e74

@devinrhode2 devinrhode2 force-pushed the breaking-createWriteStreamSync branch 7 times, most recently from 95b24eb to 1f6fd13 Compare March 6, 2022 18:37
devinrhode2 and others added 16 commits March 6, 2022 13:37
Somehow this improves coverage from this:
------------------------|---------|----------|---------|---------|-------------------
File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------------|---------|----------|---------|---------|-------------------
All files               |   38.75 |    20.83 |    37.5 |   45.45 |
 applicationinsights.js |   16.95 |        5 |   16.67 |   21.74 | 9,18-76,83-87

To:
 ------------------------|---------|----------|---------|---------|-------------------
File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------------|---------|----------|---------|---------|-------------------
All files               |   98.78 |      100 |     100 |   98.53 |
 applicationinsights.js |   98.36 |      100 |     100 |   97.92 | 91
This happens in commit b2a3c2a
    applicationinsights.ts - un-promise-ify

Does not make any sense to me 🤷
@devinrhode2 devinrhode2 force-pushed the breaking-createWriteStreamSync branch from 1f6fd13 to 830e2a8 Compare March 6, 2022 18:38
@devinrhode2 devinrhode2 marked this pull request as ready for review March 7, 2022 14:02
@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jun 12, 2022
@devinrhode2
Copy link
Author

Well that's sad
@ovhemert did you even setup/configure stale bot?

@stale stale bot removed the wontfix This will not be worked on label Jun 12, 2022
@stale
Copy link

stale bot commented Aug 13, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Aug 13, 2022
@stale stale bot closed this Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why is createWriteStream async?
1 participant