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

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

Open
julbrs opened this issue Sep 26, 2023 · 7 comments
Open

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

julbrs opened this issue Sep 26, 2023 · 7 comments

Comments

@julbrs
Copy link
Contributor

julbrs commented Sep 26, 2023

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
Copy link
Owner

ehmad11 commented Sep 26, 2023

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
Copy link
Contributor Author

julbrs commented Oct 2, 2023

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:

#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
Copy link
Contributor Author

julbrs commented Oct 3, 2023

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

OK here is the PR on suiteql: ehmad11/suiteql#15

@julbrs julbrs changed the title Handle CVE issues on got Switch project to ESM in order to cover CVE on got package Oct 3, 2023
@julbrs
Copy link
Contributor Author

julbrs commented Oct 5, 2023

@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
Copy link
Owner

ehmad11 commented Oct 7, 2023

@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
Copy link
Owner

ehmad11 commented Oct 7, 2023

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

@ehmad11 ehmad11 pinned this issue Oct 7, 2023
@julbrs
Copy link
Contributor Author

julbrs commented Oct 7, 2023

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 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants