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

ESM importing a CommonJS calls getters #35859

Closed
avaly opened this issue Oct 29, 2020 · 8 comments
Closed

ESM importing a CommonJS calls getters #35859

avaly opened this issue Oct 29, 2020 · 8 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.

Comments

@avaly
Copy link

avaly commented Oct 29, 2020

What steps will reproduce the bug?

Install pg npm dependency

Run the following module:

import pg from 'pg';

console.log(!!pg);

How often does it reproduce? Is there a required condition?

It always reproduces. The imported module needs to be a CommonJS module with getters.

In this case pg exports a getter that lazily tries to load another npm package: https://github.com/brianc/node-postgres/blob/master/packages/pg/lib/index.js#L32-L55

What is the expected behavior?

$ node -v
v14.12.0

$ node pg.mjs 
true

What do you see instead?

$ node -v
v14.13.0

$ node pg.mjs 
Cannot find module 'pg-native'
Require stack:
- /dev/temp/node-bugs/node_modules/pg/lib/native/client.js
- /dev/temp/node-bugs/node_modules/pg/lib/native/index.js
- /dev/temp/node-bugs/node_modules/pg/lib/index.js
true

Additional information

This change was introduced in #35249

@targos
Copy link
Member

targos commented Oct 29, 2020

/cc @guybedford

@targos targos added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 29, 2020
@guybedford
Copy link
Contributor

guybedford commented Oct 29, 2020

So this is by design of the implementation of ES modules in that all named export bindings must be populated upfront and can't be getters themselves. So we do call getters when populating these for the CJS module representation.

On the other hand one option might be to not detect Object.defineProperty cases to make the exports detection less accurate but also less likely to hit cases like this.

In most cases, though surely you would want this to be a named export? Is npm install pg-native to fix really that bad?

I'm also not sure we should be making breaking changes to the named exports algorithm at this stage of the game too.

So I'm somewhat on the fence if we should support this case, but it's worth considering the options.

@avaly
Copy link
Author

avaly commented Oct 29, 2020

Is npm install pg-native to fix really that bad?

If it were any other dependency, that wouldn't be an issue. But in this case, this dependency requires build tools to be installed to build a native binary (https://www.npmjs.com/package/pg-native). It's not something one should be forced to do, if we don't plan on using that part of the library.

TBH I would also like to see a change in how the pg library is exporting its native part. So maybe it's not actionable from node.js itself.

But what about other packages that may be using a similar pattern?

@guybedford
Copy link
Contributor

I've posted a fix PR in #35871 for this. I think it does make sense to ignore these exports.

@aduh95
Copy link
Contributor

aduh95 commented Nov 1, 2020

@avaly Can we close this now? Can you confirm the merged PR fixed your issue?

@avaly
Copy link
Author

avaly commented Nov 2, 2020

I've tried just now node-v16.0.0-nightly2020110209af8c822c and it shows the same behavior as 14.13.0

@guybedford
Copy link
Contributor

Thanks for checking this again, created a new PR at #35928.

@guybedford
Copy link
Contributor

Fixed in #35928. Thanks again for the quick reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
4 participants