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.

Sign up for GitHub

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

Jump to bottom

feat(): pg-query-stream typescript #2376

Merged
merged 15 commits into from
Nov 3, 2020
Merged

feat(): pg-query-stream typescript #2376

merged 15 commits into from
Nov 3, 2020

Conversation

chyzwar
Copy link
Contributor

@chyzwar chyzwar commented Oct 11, 2020 •
edited
Loading

New version should emit types compatible with
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/47e9cf6d8947e46a82bb09bbcc3dd74a3ac0ffa1/types/pg-query-stream/index.d.ts

Let me know if this is going to right direction?

TODO:

  • Confirm readable option emitClose, is this is valid option?
  • Add mocha setup for typescript
  • Convert tests to typescript
  • Ensure that emitted code is compatible with JS version

brianc
brianc reviewed Oct 13, 2020
View reviewed changes
Copy link
Owner

@brianc brianc left a comment

Choose a reason for hiding this comment

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

this looks rad - lmk if you have any questions!

packages/pg-query-stream/tsconfig.json
"module": "commonjs",
"esModuleInterop": true,
"allowSyntheticDefaultImports": true,
"strict": false,
Copy link
Owner

Choose a reason for hiding this comment

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

it will be nice to turn this to true one day, but this is fine for the initial port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this would be possible after pg-cursor is converted. I will look at pg-cursor next

@chyzwar chyzwar changed the title [WIP] pg-query-stream typescript [Draft] pg-query-stream typescript Oct 13, 2020
@chyzwar
Copy link
Contributor Author

chyzwar commented Oct 15, 2020

I think this should be ready to review.

  • Added solution project to compile pg-query-stream and pg-protocol (incremental and composite)
  • Added files in pg-query-stream and pg-protocol to avoid publishing test files and buildInfo
  • Remove emitClose from super call in QueryStream
  • Remove unsued results from _read -> read
  • Converted test to ts and changed var top consts
  • Rename class from PgQueryStream to QueryStream

@chyzwar chyzwar changed the title [Draft] pg-query-stream typescript pg-query-stream typescript Oct 15, 2020
@chyzwar chyzwar changed the title pg-query-stream typescript feat(): pg-query-stream typescript Oct 15, 2020
brianc
brianc reviewed Oct 26, 2020
View reviewed changes
packages/pg-query-stream/src/index.ts
const { batchSize, highWaterMark = 100 } = config
// https://nodejs.org/api/stream.html#stream_new_stream_readable_options
super({ objectMode: true, emitClose: true, autoDestroy: true, highWaterMark: batchSize || highWaterMark })
Copy link
Owner

Choose a reason for hiding this comment

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

curious: why remove emitClose here?

@brianc
Copy link
Owner

brianc commented Oct 26, 2020

This looks solid - thanks a lot for doing this! Sorry for the delay....have had a really horrible October - my doggie died. 😭 back in the saddle again now though.

@chyzwar
Copy link
Contributor Author

chyzwar commented Oct 27, 2020

emitClose is true by default and typescript was throwing error when It was trying to pass this option.

@chyzwar
Copy link
Contributor Author

chyzwar commented Oct 27, 2020

Sorry to hear about your dog.

@brianc
Copy link
Owner

brianc commented Nov 2, 2020

@chyzwar sorry for the delay...life is crazy. I'm down to merge this but there are a few merge conflicts. Care to take a quick look at those & resolve them? Then i'll get this merged! Thanks again! ❤️

@chyzwar chyzwar force-pushed the pg-stream-typescript branch from 886de1e to 76a5316 Compare November 2, 2020 19:53
@chyzwar
Copy link
Contributor Author

chyzwar commented Nov 2, 2020

this now has been rebased on master.

@brianc
Copy link
Owner

brianc commented Nov 3, 2020

awesome! thanks! Will get this merged up today!

@brianc brianc merged commit 78a14a1 into brianc:master Nov 3, 2020
@brianc
Copy link
Owner

brianc commented Nov 10, 2020

release pg@8.5.0

@vitaly-t
Copy link
Contributor

Added issue #2412

@vitaly-t vitaly-t mentioned this pull request Nov 14, 2020
Closed
This was referenced Mar 17, 2021
Closed
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianc brianc brianc left review comments

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chyzwar @brianc @vitaly-t @charmander