search

naugtur / npm-audit-resolver

Apache License 2.0
121 stars 28 forks source link

Support yarn #8

Closed clement-escolano closed 5 years ago

clement-escolano commented 5 years ago

Hello!

I am very interested in this project and actually use npm-audit-resolver for two projects with NPM. However I have a large number on projects running yarn and the conversion cost is too big so I worked to make your package compatible with yarn.lock file.

This works by converting the yarn.lock file into package-lock.json with synp and use the npm-audit-resolver logic to know what to do next.

What do you think about it?

I added a lot of conditions between yarn and npm in the code and it becomes a bit unclear. I will be happy to refactor the code if you think this is needed.

Thank you

naugtur commented 5 years ago

Nice work! There's one major change that needs to happen here.

This package is meant to become a part of npm itself and a lot of experimentation is ahead. I'd want this yarn support code to go into separate files and be dependency-injected into the main codebase.

So you'd create a check and resolve versions for yarn and pass all yarn or npm specific references to index.js to be propagated downwards. The upcoming week is really heavy for me at work and I might not be able to engage here, but I could scaffold this for you a bit - I know this comment is probably not enough to understand what I want to see here.

If you're not in a rush, I'm hoping we can work together to bring yarn support in.

nulltoken commented 5 years ago

This works by converting the yarn.lock file into package-lock.json with synp

Interesting approach!

clement-escolano commented 5 years ago

Nice work!

Interesting approach!

Thanks 😄

I'd want this yarn support code to go into separate files and be dependency-injected into the main codebase.

I completely understand.

So you'd create a check and resolve versions for yarn and pass all yarn or npm specific references to index.js to be propagated downwards.

I don't know exactly how to do it in Node.JS but it seems like the right way to keep the yarn part away from the the main files.

The upcoming week is really heavy for me at work and I might not be able to engage here, but I could scaffold this for you a bit - I know this comment is probably not enough to understand what I want to see here.

If you're not in a rush, I'm hoping we can work together to bring yarn support in.

I will be glad to work with you to do this. I don't know exactly where to begin but if you can show me where this has been done or what the basic approach would look like in npm-audit-resolver, I can start working on a solution on my end. There is no rush so we can work on this whenever you want.

clement-escolano commented 5 years ago

@naugtur Do you have some time this week so we can look into it together?

naugtur commented 5 years ago

I've got family visiting on top of what was going on last week, so probably not. I'm planning an opensource sprint some time in the week of Feb 25. You in?

On Feb 11, 2019, 17:14, at 17:14, "Clément Escolano" notifications@github.com wrote:

@naugtur Do you have some time this week so we can look into it together?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/naugtur/npm-audit-resolver/pull/8#issuecomment-462387970

clement-escolano commented 5 years ago

I'm planning an opensource sprint some time in the week of Feb 25. You in?

@naugtur Perfect :wink:

clement-escolano commented 5 years ago

@naugtur Hello 😄 Do you have time this week to work on this? Have a good week

naugtur commented 5 years ago

Looks like I might. I wanted to consult one idea with you.

So I think at least initially the part that runs as check-audit is more likely to land in npm. I was thinking I could make a cleaner split between the two parts and then we'd focus on porting them to also support yarn separately. Which one should we start with in your opinion? The checker or the interactive resolver?

clement-escolano commented 5 years ago

I think the checker will be simpler to port to yarn because the interactive resolver has a lot of actions (remove, install, upgrade) that needs to be converted to yarn. So I think we should start with the checker. What do you think?

naugtur commented 5 years ago

Ok, I agree. So tomorrow on the train I'll start the work of introducing some abstractions behind the checker.

clement-escolano commented 5 years ago

OK perfect. If I can help you, feel free to ask!

naugtur commented 5 years ago

I'm working on refactoring - you can follow the changes on a branch. For now I made a mess :) Hope to get it to a better state soon. I'll let you know when I think is a good time for you to come in, but feel free to comment earlier.

clement-escolano commented 5 years ago

Hello @naugtur Have you made progress on the refactoring part? There is no rush of course, I just want to know what is the status on it. Thanks!

naugtur commented 5 years ago

Not long after my last comment my kid got into a hospital and life got postponed. I'll get back to it no sooner than later next week I think

clement-escolano commented 5 years ago

@naugtur I am sorry to hear that. Take the time you need, I hope the situation will get better.

jim-moody commented 5 years ago

Any update on this? Our team would also benefit from yarn support

naugtur commented 5 years ago

I'm working on the refactoring in rare spare time. I'm 50% through. I'll comment back in this thread once refactored, with instructions on how to proceed - at that point yarn support should be easy to add.

naugtur commented 5 years ago

I think I fi ished rewriting the core today. Now it's time to get down to the part that uses specific commands. No call to action for you yet, just letting you know I'm finally coming through with it. Check out the refactoring branch

clement-escolano commented 5 years ago

OK thank you for the heads up!

naugtur commented 5 years ago

@clement-escolano I just pushed to the refactoring branch. All places you need to modify to add yarn support are marked with //MARK_YARN

You can start a new branch from the current state and implement, I'm not expecting conflicts. There's some work I need to do on the interactive part, but the rest is mostly done. Feel free to close this PR and opening new if repurposing it is too uncomfortable.

If you're done by the end of the week, you should make it into the big release.

naugtur commented 5 years ago

Hey @clement-escolano - I know I've been unresponsive before and I do not mean to pressure you in any way. I'm intending to release a totally new version of npm-audit-resolver and I was hoping it'd support yarn. Please let me know if you'll be able to cooperate on this. If not, I'll introduce basic yarn support on my own based on your PR here.

clement-escolano commented 5 years ago

@naugtur Hello! No problem, yes I will be glad to help you on it. Which branch should I look at?

naugtur commented 5 years ago

@clement-escolano The branch is called refactoring. Look for the //MARK_YARN bits

naugtur commented 5 years ago

Hey @clement-escolano - I'm finishing work on v2 and hoping to publish it soon. Let me know if you can adapt this to v2 in refactoring branch this weekend, or if I should do it.

I'll do my best to credit you for the solution but would prefer to see you in all automated contributor listings :)

If I implement yarn support I'll probably leave room for improvement.

naugtur commented 5 years ago

so I rewrote this into v2 codebase. https://github.com/naugtur/npm-audit-resolver/pull/15 Haven't tested anything yet. Let me know if you want to continue being involved in it.