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

Upgrade chalk dep and rewrite code to use ESM #66

Open
wants to merge 1 commit into
base: npm7-dev
Choose a base branch
from

Conversation

jesse-mm
Copy link

@jesse-mm jesse-mm commented Apr 8, 2023

Hi @naugtur this PR updates npm7 release branch and hopefully addresses your last point: upgrading packages.

I did an upgrade of chalk, chalk is now an only ESM package. I've updated the codebase and removed all the require (commonjs) imports and changed them to ESM.

All tests seem to run fine! Another small change to the license field as it was complaining that is was not SPDX compatible. So I've set the correct license identifier for Apache 2.0.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get rid of this file, as all imports are changed to 'audit-resolve-core/arguments.js'

@naugtur
Copy link
Owner

naugtur commented Apr 9, 2023

I appreciate the effort and interest in my humble project. Thank you.

The current state of the branch is a release candidate and I've deliberately avoided updating chalk all the way to not destabilize the codebase with a refactor right before a release. It's been waiting for some more testing and finally, some attention from me to make the final release.

I didn't want to switch to esm to be honest. Let it be a lesson in how to interact in opensource - I'd let you know if you asked.
The best course of action, if you wish to contribute to a project in OSS is to first create an issue or a draft PR with a description of what you want to do and ask if all of it are welcome changes.

I will not put your changes in the v3.0.0 major release, but I don't want them to go to waste.

We can look at the esm refactor and merge that if it adds value.
Here are also other changes - the init function dynamically added to the facade. I'm not sure what the purpose is of that change. Are you considering initializing more than once per process lifetime?

Anyway, I'm glad you want to help and of we can align our goals I'd be happy to bring you in as a contributor if that's something you want to do.

@jesse-mm
Copy link
Author

jesse-mm commented Apr 9, 2023

I appreciate the effort and interest in my humble project. Thank you.

It's great project! I'm trying to integrate it within our team tooling. As you know we are dealing with a lot of false positives with NPM audit. And we want to backtrack and log why we are "ignoring" certain sec issues.

The current state of the branch is a release candidate and I've deliberately avoided updating chalk all the way to not destabilize the codebase with a refactor right before a release. It's been waiting for some more testing and finally, some attention from me to make the final release.

This makes totally sense if you look at the amount of changes to make it work within ESM.

I didn't want to switch to esm to be honest. Let it be a lesson in how to interact in opensource - I'd let you know if you asked.
The best course of action, if you wish to contribute to a project in OSS is to first create an issue or a draft PR with a description of what you want to do and ask if all of it are welcome changes.

Got it and totally understand (me as random stranger dumping a shitload of potentially breaking changes 😉 ). I'll file an issue/ticket the next time so we can collaborate and discuss if those changes are welcome :).

I will not put your changes in the v3.0.0 major release, but I don't want them to go to waste.

This is fine! And lets have a look, but I'm totally fine if you decline the changes. It was good to get a view on how the project is setup.

We can look at the esm refactor and merge that if it adds value.

Sure I think looking towards the future as the NPM ecosystem moves more to ESM, It will be beneficial to keep all the project deps updated. But I understand that would also mean a rewrite for audit-resolve-core.

Here are also other changes - the init function dynamically added to the facade. I'm not sure what the purpose is of that change. Are you considering initializing more than once per process lifetime?

This was to keep consistent with the existing style of the codebase, as require is sync I've changed the loading of the pkgmanager modules. But as import is async I needed to await the loading of these modules therefore I created init . In hindsight they could have been just added as top level imports.

Anyway, I'm glad you want to help and of we can align our goals I'd be happy to bring you in as a contributor if that's something you want to do.

🙌 Yes I'm open to contributing to your project, and help you with some of the tasks that are on the roadmap.

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

Successfully merging this pull request may close these issues.

2 participants