oracle / node-oracledb

Oracle Database driver for Node.js maintained by Oracle Corp.
http://oracle.github.io/node-oracledb/
Other
2.24k stars 1.07k forks source link

Unification nodejs imports #1559

Closed sla100 closed 1 year ago

sla100 commented 1 year ago

Hello. Congratulations on working on the new "thin" version!. I allowed myself a PR unifying the way of importing dependencies from nodejs. Currently, the design uses a mixed style. Sometimes these are references to global objects, sometimes imports. IMHO there are preferred:

Best regards.

Signed-off-by: Sławomir Osoba <sla100@poczta.fm>
oracle-contributor-agreement[bot] commented 1 year ago

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA). The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

cjbj commented 1 year ago

Thanks @sla100 !

Some of our new code has been itching @anthony-tuininga for a while and he is about to commit a big cleanup and simplification. I'll let him comment on your PR and how it can be integrated.

anthony-tuininga commented 1 year ago

Thanks, @sla100. Unless you have a good reason to do otherwise, I think I'll use this approach instead:

const { Buffer } = require('buffer');

I'll make sure this gets integrated with the big cleanup. Thanks again!

sla100 commented 1 year ago

@anthony-tuininga

I think I'll use this approach instead

Sure. It is closer to esm import statement.

I can also suggest some modification for .eslintrc. Node option are heavy and allow to use many global references.

"env": {
  "node": true,
}

After being replaced by:

"env": {
  "shared-node-browser": true,
  "commonjs": true,
}

allow only references accomplished to ESM standards and cjs require / exports.

cjbj commented 1 year ago

@sla100 changes landed in https://github.com/oracle/node-oracledb/commit/15f10c150919bbbc36357b27a5da0c43a5d92f2f and https://github.com/oracle/node-oracledb/commit/ae623d1e8c9bf637966396710d4b3f30c4cef3e0. Thanks for the advice and contribution.

[For other readers not familiar with our PR process, we take the code from PRs into a private repo (which we need in order to work on unreleased Oracle DB feature support) and then the PR changes get committed back to GitHub.]