Make `native` non-enumerable #1992

gabegorelick · 2019-10-17T18:36:29Z

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.

gabegorelick · 2019-10-17T18:55:58Z

Test failures are from tests that are flaky on master.

gabegorelick · 2019-10-17T18:59:19Z

lib/index.js

-        throw err
+  var native
+  Object.defineProperty(module.exports, 'native', {
+    enumerable: false,


The use of a get here implies writable: false, and Object.defineProperty defaults to configurable: false. Any of those could be breaking changes, depending on how clients are interactive with native (are they overwriting it?).

lib/index.js

charmander · 2019-10-18T01:42:33Z

Cloning or iterating over pg is a very strange thing to do to begin with… not that I think this change shouldn’t go in.

gabegorelick · 2019-10-18T01:51:30Z

Cloning or iterating over pg is a very strange thing to do to begin with

Unintentional cloning is pretty common in ORMs like Sequelize where models have a reference to DB connections. So making a deep copy of a model instance ends up enumerating pg instances. See sequelize/sequelize#3781.

But cloning a pg instance directly? I agree that is strange.

I think a more likely use case that this PR could break is consumers overriding or deleting native, perhaps to avoid the console.log. Unfortunately, this change will cause such use cases to throw an error.

1valdis · 2019-10-28T04:29:57Z

Can we have this merged and released ASAP, please? In sequelize, there are weird errors cannot find module "pg-native" happening at seemingly random situations and I couldn't find the root cause...

charmander · 2019-10-28T17:34:15Z

@1valdis That’s probably a bug you or Sequelize should fix, not hide. Deep-cloning clients is a bad idea.

gabegorelick · 2019-10-31T14:26:01Z

Any chance this will get merged?

brianc · 2019-11-11T18:33:06Z

yah we try to be as pedantic as possible about breaking changes and this is technically a non-backwards compatible change. I agree it's super bizarre to enumerate the pg instance but folks do bizarre things sometimes.

It's also a small thing, but i'd like to see at least a simple unit test that does something like

assert.equals(Object.keys(pg).includes('native'), false)

or something just to future-proof against this changing.

I also totally agree w/ charmander...if code is 'randomly' throwing an error about pg-native not being installed it's nothing to do w/ this module directly (though making the prop non-enumerable will probably help those situations) because pg doesn't do anything randomly wrt requring native so it's the consuming module's responsibility to fix that random error, not here.

gabegorelick · 2019-11-13T15:19:26Z

yah we try to be as pedantic as possible about breaking changes and this is technically a non-backwards compatible change. I agree it's super bizarre to enumerate the pg instance but folks do bizarre things sometimes.

@brianc Does that mean this PR should be reworked to be backwards compatible, or that you're fine releasing a new major version of node-pg? I'm not sure there's an easy way to make it backwards compatible. Instead we may have to introduce something like #1894 that allows consumers to opt-in to this change.

brianc · 2019-11-19T16:14:26Z

I think leaving this as non-backwards compatible is fine. I need to do a semver major bump soon and I'll sweep it into those changes when I do that. If things pan out well I'll be able to do it this weekend, but if not definitely in the next few weeks. Holidays + lots of travel make things extra slow...but I appreciate the work and will release it ASAP!

1valdis · 2019-11-20T08:24:08Z

Awesome news. Waiting for the release, hoping that Sequelize won't break with major version change of pg. Is there a (preliminary) list of breaking changes?

brianc · 2019-11-20T18:55:41Z

not yet w/ the list of breaking changes but will be coming soon as soon as I have a chance to put it together. :)

…

On Wed, Nov 20, 2019 at 2:24 AM Vladyslav Huba ***@***.***> wrote: Awesome news. Waiting for the release, hoping that Sequelize won't break with major version change of pg. Is there a (preliminary) list of breaking changes? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1992?email_source=notifications&email_token=AAAMHIJGX3KF6WM6XBKETOTQUTX2RA5CNFSM4JB5T6NKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEREKLA#issuecomment-555894060>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHIOWVB3YXMFR5Q65DZTQUTX2RANCNFSM4JB5T6NA> .

lib/index.js

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 brianc#1894 (comment)

gabegorelick · 2020-01-03T22:32:02Z

Rebased to fix merge conflicts, but otherwise no changes were made.

@brianc Let me know if there's anything I can help with to get this merged and released.

brianc · 2020-01-03T22:52:04Z

Will do! I cut my fingertip off on NYE so cant really type (1 hand all bandaged) - i expect to be back in service on Monday at the latest and will review and merge in an 8.0 branch shortly! Wanna get 8.0 out asap so I can start on some major refactoring for 9.0!! Thanks for the PR! ❤️

…

On Fri, Jan 3, 2020 at 4:32 PM Gabe Gorelick ***@***.***> wrote: Rebased to fix merge conflicts, but otherwise no changes were made. @brianc <https://github.com/brianc> Let me know if there's anything I can help with to get this merged and released. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1992?email_source=notifications&email_token=AAAMHIMGEOXFNJ7YBGEU6HLQ364GHA5CNFSM4JB5T6NKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEICHPPY#issuecomment-570718143>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHINMYP22YUBKKI4Q7BTQ364GHANCNFSM4JB5T6NA> .

charmander · 2020-01-15T22:30:35Z

Landed on the 8.0 branch with a test in #2065.

michaelbromley · 2020-01-27T12:19:06Z

I agree it's super bizarre to enumerate the pg instance but folks do bizarre things sometimes. #1992 (comment)

I've just been chasing down this issue and I think in my case it is caused by TypeScript's implementation of ES module imports:

https://github.com/microsoft/tslib/blob/3cf8ae045d949fe7a636d8274d0974a4f30ede8d/tslib.js#L227-L233

    __importStar = function (mod) {
        if (mod && mod.__esModule) return mod;
        var result = {};
        if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
        result["default"] = mod;
        return result;
    };

My use-case is using pg in a Jest test file written in TypeScript. I look forward to the 8.0 release!

gabegorelick force-pushed the nonenumerable-native branch 3 times, most recently from 41c411a to 32ba854 Compare October 17, 2019 18:49

gabegorelick commented Oct 17, 2019

View reviewed changes

lib/index.js Outdated Show resolved Hide resolved

papb mentioned this pull request Oct 25, 2019

Error: Cannot find module 'pg-native' sequelize/sequelize#3781

Closed

gabegorelick mentioned this pull request Oct 25, 2019

Ability to force *not* loading native client #1894

Closed

brianc added this to the [email protected] milestone Nov 11, 2019

charmander requested changes Dec 6, 2019

View reviewed changes

lib/index.js Outdated Show resolved Hide resolved

lib/index.js Outdated Show resolved Hide resolved

lib/index.js Outdated Show resolved Hide resolved

gabegorelick force-pushed the nonenumerable-native branch 2 times, most recently from 07f44a7 to 18a8e14 Compare December 6, 2019 15:57

charmander approved these changes Dec 6, 2019

View reviewed changes

Make native non-enumerable

45a427e

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 brianc#1894 (comment)

gabegorelick force-pushed the nonenumerable-native branch from 18a8e14 to 45a427e Compare January 3, 2020 22:18

brianc mentioned this pull request Jan 10, 2020

Make native non-enumerable #2065

Merged

charmander closed this Jan 15, 2020

gabegorelick deleted the nonenumerable-native branch January 15, 2020 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `native` non-enumerable #1992

Make `native` non-enumerable #1992

gabegorelick commented Oct 17, 2019

gabegorelick commented Oct 17, 2019

gabegorelick Oct 17, 2019

charmander commented Oct 18, 2019

gabegorelick commented Oct 18, 2019

1valdis commented Oct 28, 2019

charmander commented Oct 28, 2019

gabegorelick commented Oct 31, 2019

brianc commented Nov 11, 2019

gabegorelick commented Nov 13, 2019

brianc commented Nov 19, 2019

1valdis commented Nov 20, 2019

brianc commented Nov 20, 2019 via email

gabegorelick commented Jan 3, 2020

brianc commented Jan 3, 2020 via email

charmander commented Jan 15, 2020

michaelbromley commented Jan 27, 2020

Make native non-enumerable #1992

Make native non-enumerable #1992

Conversation

gabegorelick commented Oct 17, 2019

gabegorelick commented Oct 17, 2019

gabegorelick Oct 17, 2019

Choose a reason for hiding this comment

charmander commented Oct 18, 2019

gabegorelick commented Oct 18, 2019

1valdis commented Oct 28, 2019

charmander commented Oct 28, 2019

gabegorelick commented Oct 31, 2019

brianc commented Nov 11, 2019

gabegorelick commented Nov 13, 2019

brianc commented Nov 19, 2019

1valdis commented Nov 20, 2019

brianc commented Nov 20, 2019 via email

gabegorelick commented Jan 3, 2020

brianc commented Jan 3, 2020 via email

charmander commented Jan 15, 2020

michaelbromley commented Jan 27, 2020

Make `native` non-enumerable #1992

Make `native` non-enumerable #1992