ehmad11 / netsuite-rest

Make requests to Oracle NetSuite REST Web Services.
MIT License
50 stars 18 forks source link

Switch project to ESM in order to cover CVE on got package #27

Open julbrs opened 12 months ago

julbrs commented 12 months ago

Hi @ehmad11 There is some CVE on the got package.

Are you available to merge and publish if I suggest a PR to handle this issue ?

Thanks!

ehmad11 commented 12 months ago

hi @julbrs,

Yes, sure, but the latest got package itself is now native ESM and no longer provides a CommonJS export. So, ideally, we need to update this library too.

julbrs commented 11 months ago

Hi @ehmad11!

Thanks for your quick feedback! I have worked a bit on netsuite-rest to migrate to ESM (guide suggested here), so it's possible:

https://github.com/ehmad11/netsuite-rest/pull/28

But then any project that is relying on netsuite-rest will break...

julien@LMON0015 utils % DEBUG=* node netsuite_import_test_data.js
/xx/netsuite_import_test_data.js:1
const NsApiWrapper = require('netsuite-rest')
                     ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /xx/netsuite-rest.js from /xx/netsuite_import_test_data.js not supported.
Instead change the require of netsuite-rest.js in xx/netsuite_import_test_data.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/xxx/netsuite_import_test_data.js:1:22) {
  code: 'ERR_REQUIRE_ESM'
}

What is the best option here? Force ESM migration for all ? I am a newbie on CJS/ESM stuff 😅 thinking of starting a brand new TS project for that 🤓

julbrs commented 11 months ago

I will now get a look at the suiteql package that is depending on this one,

OK here is the PR on suiteql: https://github.com/ehmad11/suiteql/pull/15

julbrs commented 11 months ago

@ehmad11 I can take some resp. / ownership on both projects if you don't have time / access to a Netsuite system to run tests. Let me know! I would like also to provide types (so probably migrate the projects to TS !)

ehmad11 commented 11 months ago

@julbrs, just saw your package; it looks great. I am thinking, can we keep this current package as it is? Many projects depend on it, and switching to ESM would be a breaking change for them. We can put a deprecated notice in this package and advise users to use your package for future updates. I would really appreciate it if we could keep the new package as a fork of the current repo to preserve the commit history and contributions of other members with full credits.

ehmad11 commented 11 months ago

and yes we can merge the suiteql, it is a great idea

julbrs commented 11 months ago

That's a very good idea!

I have take rebuilt the new repository here: https://github.com/julbrs/netsuite-api-client which is a github fork of https://github.com/ehmad11/netsuite-rest ( because there is more contribution in this one). There is just one more commit with all the new stuff (esm/typescript/merging).

I have also added a specific section in the README to explain the ESM/CommonJS stuff, with links to netsuite-rest and suiteql if user want to grab the CommonJS version on npmjs.

Thanks !