-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
pg-query-stream version + compatibility #2412
Comments
@brianc I made that change. I saw that tests, documentation and definitely typed where using QueryStream as name. Let me know what do you think ? |
@chyzwar I think we should keep the class name as is, just up |
That doesn’t sound like a good idea. |
Yeah there's an argument that you could say any change is a breaking change. What if an external library validated a function by the contents of calling I'm fine to bump the version if there's a material issue caused by this, but I'd like to see an actual example in the wild before just calling this a breaking change because the name of a class changed. It sounds more like pedantry than an actual issue. |
@brianc Your Class name is the only approach I found usable. If you have a better way, then please do post it here. |
I would imagine something like... const QueryStream = require('pg-query-stream')
if (something instanceof QueryStream) {
// we have a stream!
} that also works like this const ICanCallThisAnythingIWant = require('pg-query-stream')
if (something instanceof ICanCallThisAnythingIWant) {
// we have a stream!
} Does that work? |
@brianc Nope, it doesn't. The modern NodeJS has gotten too progressive, one might say. These days you have things like Another issue - you have to include the actual module type into your project. But when you only add support for it, you do not really need its implementation, makes it way more flexible. And avoids versioning conflict. |
Sorry - not following what you're saying about docker - how can you require modules from one docker container into another? I don't do super advanced stuff w/ docker. Also, not following what you mean about including the actual module type in your project, could you show me a code sample or example here? I still think checking the |
Within docker, it is possible to load the same module multiple times, each within isolated environment, so if you create a class object in one, pass it into the other, then
When you want to add support for
I have explained the best I could why and why it doesn't work, so not much option there.
Yes, please, it would make life easier, since the last version suddenly changed the class name. |
pg-query-stream
has been fully revamped in the latest release, with at least one breaking change - changing the class name again, fromPgQueryStream
toQueryStream
(see PR #2476).There are libraries out there that validate the object by its class name, so those would all need to be updated.
The request is here that
pg-query-stream
version should have been upped to 4.0.0, or not with the minor version increment.Please, either bring the version to 4.0.0 (preferable), or rename the class back to
PgQueryStream
(this will require TypeScript update as well, so the first option is better).Note that this is the second time such a change happens. Which means there should be a test added for the class name, to make sure people stop changing it. Internal class names are often important, being the only way an object instance can be validated.
The text was updated successfully, but these errors were encountered: