Drop support for EOL versions of node #2062
Conversation
|
As per my comments from #2068...
|
|
Continuing discussion from #2062 - should we continue to support [email protected]? It's a few years old and end of life so not supported anymore by node.js itself...however, I don't want to lock out people on old versions of node from upgrades without a reason. Node prior to 8 has definitely caused issues so that's going bye bye from the support matrix...but if all the tests still pass on 8...maybe we set the minimum version to 8 unless there's a compelling reason to drop it? I think one of the ideas for dropping 8 was |
|
Okay yeah...thanks for weighing in on this @vitaly-t - I think you've convinced me to keep supporting v8.x of node until there's a compelling reason not to but I'll wait over the weekend to let others weigh in as well. ❤️ you're right it's a lot more common to run EOL versions of node in prod than it can seem when you're working at a bleeding edge startup or just on side projects using the newest version of everything. |
|
The reason many companies are stuck with older Node.js is having to use a large pool of packages that do not yet support newer Node.js versions, and you can use only one version of Node.js at a time, so it becomes a necessary compromise to carry on with an older Node.js |
|
I think the maintenance burden is a good enough reason to drop support for older Node.js versions, especially when a project is open-sourced. The more legacy patterns remain in this project the harder it is to maintain, not only for the core maintainers but also for community members wishing to submit PRs. When a maintainer of an open source project is willing to keep working on a project, I think it's in our best interests to support that maintainer however we can. If, as in this case, dropping support for older Node.js versions makes maintenance easier for them then it is what it is. The benefits for new users are increased (with new features and "newer", perhaps easier to understand code), while for users on older versions, the code will still keep working. That said, important bug fixes can still be backported to the older versions, which is something that can also be pioneered by the consumers of the older versions due to the open-source nature of it all. |
You confuse working for community with writing some code for your own amusement. Countless businesses depend on this library. It will either have support done right, or it will be abandoned and forgotten. |
|
Yeah im fine keeping support for node 8. At the end of the day _any_ amount
of maintenance can be viewed as a burden __or__ a labor of love. I choose
to see it as a joy and privilege to get to maintain this. So...I am fine
with keeping 8 support for now. I plan on transitioning to typescript for
pg@9 and that will help smooth over any JS inconsistency between node
versions too.
I will say to the countless businesses relying on this module...you should
sponsor me!
https://github.com/sponsors/brianc
❤️
Ill change this PR to support node>=8 by adding it back to the test matrix
later today and then merge it!
…On Sun, Jan 12, 2020 at 11:08 AM Vitaly Tomilov ***@***.***> wrote:
When a maintainer of an open source project is willing to keep working on
a project, I think it's in our best interests to support that maintainer
however we can. If, as in this case, dropping support for older Node.js
versions makes maintenance easier for them then it is what it is
You confuse working for community with writing some code for your own
amusement. Countless businesses depend on this library. It will either have
support done right, or it will be abandoned and forgotten.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2062?email_source=notifications&email_token=AAAMHIJG2UIYP25VMUCTFHDQ5NE7DA5CNFSM4KFJSWD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIW675A#issuecomment-573435892>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAMHIMY5MBYUZX4FAMYS2LQ5NE7DANCNFSM4KFJSWDQ>
.
|
|
Okey dokey re-added node8.x to the test matrix....so it'll still be supported for maybe another year or so or until it becomes a blocker on shipping some new feature? We'll figure it out some time in the future! 🚀 |
|
(There are still a few changes to .travis.yml that weren’t reverted) |
yeah I just left one of the carbon runs out - its testing against a diff minor version of postgres but we test against that version w/ another version of node...so I figured it was a bit redundant. I can add it back if you think it's important...but in reality there are so few changes in minor versions of postgres anyway its prolly overkill. |
|
What that one exists to test, IIRC, is Ubuntu 12.04 with the most recent Postgres it supports. (The other |
|
cool - i put it back! |
Co-Authored-By: Charmander <[email protected]>
* Drop support for EOL versions of node * Re-add testing for [email protected] * Revert changes to .travis.yml * Update packages/pg-pool/package.json Co-Authored-By: Charmander <[email protected]> Co-authored-by: Charmander <[email protected]>
* Drop support for EOL versions of node * Re-add testing for [email protected] * Revert changes to .travis.yml * Update packages/pg-pool/package.json Co-Authored-By: Charmander <[email protected]> Co-authored-by: Charmander <[email protected]>
* Drop support for EOL versions of node (#2062) * Drop support for EOL versions of node * Re-add testing for [email protected] * Revert changes to .travis.yml * Update packages/pg-pool/package.json Co-Authored-By: Charmander <[email protected]> Co-authored-by: Charmander <[email protected]> * Remove password from stringified outputs (#2066) * Remove password from stringified outputs Theres a security concern where if you're not careful and you include your client or pool instance in console.log or stack traces it might include the database password. To widen the pit of success I'm making that field non-enumerable. You can still get at it...it just wont show up "by accident" when you're logging things now. The backwards compatiblity impact of this is very small, but it is still technically somewhat an API change so...8.0. * Implement feedback * Fix more whitespace the autoformatter changed * Simplify code a bit * Remove password from stringified outputs (#2070) * Keep ConnectionParameters’s password property writable `Client` writes to it when `password` is a function. * Avoid creating password property on pool options when it didn’t exist previously. * Allow password option to be non-enumerable to avoid breaking uses like `new Pool(existingPool.options)`. * Make password property definitions consistent in formatting and configurability. Co-authored-by: Charmander <[email protected]> * Make `native` non-enumerable (#2065) * Make `native` non-enumerable Making it non-enumerable means less spurious "Cannot find module" errors in your logs when iterating over `pg` objects. `Object.defineProperty` has been available since Node 0.12. See #1894 (comment) * Add test for `native` enumeration Co-authored-by: Gabe Gorelick <[email protected]> * Use class-extends to wrap Pool (#1541) * Use class-extends to wrap Pool * Minimize diff * Test `BoundPool` inheritance Co-authored-by: Charmander <[email protected]> Co-authored-by: Brian C <[email protected]> * Continue support for creating a pg.Pool from another instance’s options (#2076) * Add failing test for creating a `BoundPool` from another instance’s settings * Continue support for creating a pg.Pool from another instance’s options by dropping the requirement for the `password` property to be enumerable. * Use user name as default database when user is non-default (#1679) Not entirely backwards-compatible. * Make native client password property consistent with others i.e. configurable. * Make notice messages not an instance of Error (#2090) * Make notice messages not an instance of Error Slight API cleanup to make a notice instance the same shape as it was, but not be an instance of error. This is a backwards incompatible change though I expect the impact to be minimal. Closes #1982 * skip notice test in travis * Pin [email protected] for regression in async iterators * Check and see if node 13.8 is still borked on async iterator * Yeah, node still has changed edge case behavior on stream * Emit notice messages on travis * Revert "Revert "Support additional tls.connect() options (#1996)" (#2010)" (#2113) This reverts commit 510a273. * Fix ssl tests (#2116) * Convert Query to an ES6 class (#2126) The last missing `new` deprecation warning for pg 8. Co-authored-by: Charmander <[email protected]> Co-authored-by: Gabe Gorelick <[email protected]> Co-authored-by: Natalie Wolfe <[email protected]>
+100 on this 😊 If your business gets some value out of this, return the favor and provide some vale to @brianc ! Also, just adding a +1 that node 8 support is still useful for our team, so I'm glad you kept it around! We're in the process of transitioning up to node 10 or 12 on a few repos, but not there quite yet. |
|
Thanks @karlbecker ❤️ Glad to hear leaving node 8 support was the way to go! It'll be supported in |
Starting to work on the 8.0 release now! First things first: drop support for EOL versions of node. This particular PR doesn't do much outside of removing node 8 from the test matrix, and removing a few places where we check the version of node and branch for earlier versions. More importantly, this will allow us to start using newer features like
async/awaitinternally which is big!I'm targeting all pull requests into the
bmc/8.0branch. Once that branch looks good we can release it, and then I'll get to work on 9.0 stuff which is a lot more significant internal changes in the name of performance & maintainability.