search

snyk / nodejs-lockfile-parser

Generate a Snyk dependency tree from package-lock.json or yarn.lock file
Other
59 stars 28 forks source link

feat: add support for yarn 2 lock files #56 #57

Closed regevbr closed 4 years ago

regevbr commented 4 years ago

What this does

Fixes #56 Allows parsing of yarn 2 lock files, which are different from yarn 1

Notes for the reviewer

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

regevbr commented 4 years ago

@orsagie @lili2311 @FauxFaux @miiila can you please CR this?

lili2311 commented 4 years ago

Hi @regevbr 👋 Thank you was this awesome contribution, we have many team members on holiday today so we would like to discuss this internally with them when they are back tomorrow and get back to you.

regevbr commented 4 years ago

@lili2311 how are you?

Can you please let me know when you are expected to fix the yarn2 issue? this is the only reason I can't migrate my projects to use yarn2...

regevbr commented 4 years ago

@arcanis I know you are a very busy man, but I was wondering if you can take a quick look into my PR here to see if I have done it right and if maybe there is a better way to achieve it? I thought about resuing the code from each relevant plugin but they are too coupled to the actual resolving logic.

regevbr commented 4 years ago

@arcanis thanks for the review! much appreciated :-) I will fix all your comments

dkontorovskyy commented 4 years ago

@regevbr that is really nice contribution. I will pull it to test locally first and then let's try to merge it. If I will need any of your help, are you up to pairing in close future?

regevbr commented 4 years ago

@dkontorovskyy sure thing! anything you need. Thanks!

regevbr commented 4 years ago

I rebased as there were some changes made to master. I also synced the tree size limits logic to the base yarn parser and added the relevant test to yarn2 parser test

dkontorovskyy commented 4 years ago

Hey @regevbr. I run local tests on your PR and it looks great 🎉 . I would like to make a graceful roll out for yarn v2, so it would be great if we can pair and separate this PR in 2 smaller bits and take it 1 by 1 into this plugin. I would like to schedule a pairing session with you whenever you free. What do you think about that?

regevbr commented 4 years ago

@dkontorovskyy thanks! Sure we can do a session, I'm generally available 12am-3am GMT+3 How do you want to split it?

dkontorovskyy commented 4 years ago

@regevbr luckily we are in the same timezone 😃 I can do 12AM-2PM tomorrow. Want me to send you an invite to zoom meeting? How can I contact you ? I would like to split it into two: first one is refactor needed for a new feature and second one is actually yarn2 support that will be easily added once the first one is in.

dkontorovskyy commented 4 years ago

@regevbr just keeping update public 😃 . We are looking to plan this feature properly this sprint and start active development next sprint. This is an amazing contribution, so we would try to bring it into the core of Snyk as soon as possible, we just need to make sure this can be done harmlessly to the overall system 🙏

lili2311 commented 4 years ago

https://github.com/snyk/nodejs-lockfile-parser/issues/56

dkontorovskyy commented 4 years ago

@regevbr first refactor PR went in 💪

regevbr commented 4 years ago

@dkontorovskyy that's good news! I will rebase this branch so we will be ready for the next phasse

dkontorovskyy commented 4 years ago

@arcanis using @yarnpkg/core in module yields tsc error 

node_modules/@types/emscripten/index.d.ts:53:33 - error TS2304: Cannot find name 'WebGLRenderingContext'.
53     preinitializedWebGLContext: WebGLRenderingContext;
                                   ~~~~~~~~~~~~~~~~~~~~~
node_modules/@types/emscripten/index.d.ts:67:28 - error TS2304: Cannot find name 'MessageEvent'.
67     onCustomMessage(event: MessageEvent): void;

I see that there was an addition to package.json of @yarnpgk/libzip to use @types/emscripten as production dependencies, because of the bug (https://github.com/yarnpkg/berry/issues/746).

Loos like node.js doesn't like WebAssembly types.

Are you aware of any workarounds for that except silencing TSC compiler with --skipLibCheck ?

regevbr commented 4 years ago

@dkontorovskyy this is solved by adding "dom" as a lib to tsconfig:

        "lib": [
            "es2017",
            "dom"
        ],

I tried getting the library to compile without skipping the lib check, and it requires a lot of changes as there are multiple errors originating from different modules. The only errors left comes from d.ts reference issues in @yarnpkg/core. For example:

node_modules/@yarnpkg/core/lib/structUtils.d.ts:91:66 - error TS2307: Cannot find module '../../yarnpkg-fslib/sources'.

91 export declare function slugifyLocator(locator: Locator): import("../../yarnpkg-fslib/sources").Filename;

I will open a ticket to fix it in yarn. You can check out all the changes I made in the next commit to this branch and let me know if that is that important for you to not skip the lib check.

regevbr commented 4 years ago

@dkontorovskyy I have worked with @arcanis and the issue is now fixed! There is a new issue in clipanion which I will try to get resolved soon as well

regevbr commented 4 years ago

@dkontorovskyy I fixed the issue in clipanion and now the build works perfectly! How do we proceed?

dkontorovskyy commented 4 years ago

@regevbr that is so cool!! What was the issue and fix?

regevbr commented 4 years ago

@dkontorovskyy in clipanion? see https://github.com/arcanis/clipanion/issues/25 for more details, in high level, the tool used for minifying the d.ts file has bugs in it causing a faulty d.ts file.

If you are asking about the issue in @yarnpkg/core than it is very complicated you can check out my summary at https://github.com/yarnpkg/berry/issues/1278#issuecomment-640903243

dkontorovskyy commented 4 years ago

@regevbr I merged your PR in our separate feature branch to run some final round of investigation we are doing with the team today. Will keep you posted 🤗

regevbr commented 4 years ago

Thanks @dkontorovskyy! Looking forward to hear back from you

regevbr commented 4 years ago

@dkontorovskyy! how's it going? I see that the feature was finally merged! does that mean I can enable snyk in my yarn2 packages now (ci and PR guard)?

regevbr commented 4 years ago

@dkontorovskyy! how's it going? I see that the feature was finally merged! does that mean I can enable snyk in my yarn2 packages now (ci and PR guard)?

dkontorovskyy commented 4 years ago

Hi @regevbr, sorry for spacing out!

It should work in CLI now. Just try scanning your project locally or in CI. Not in PR guard yet. My team will be looking soon into this.

regevbr commented 4 years ago

That's great news! Please let me know when the guard is working as well as we use it as well :-)

kachkaev commented 3 years ago

Still getting parse errors in a Github PR here: https://github.com/kachkaev/njt/pull/29

Screenshot 2020-12-24 at 11 35 29

What are the current plans to resolve this?

I had to replace Snyk integration with running yarn audit on schedule in another project because it was blocking the PR merge. Looking forward to bring Snyk back when CI integration is finally working!

cc @arcanis

kachkaev commented 3 years ago

👋 from August 2021 😅

https://github.com/kachkaev/njt/pull/29

Screenshot 2021-08-07 at 12 45 52 Screenshot 2021-08-07 at 12 46 11
kachkaev commented 2 years ago

UPD: 👋 from November 2021 😅

https://github.com/snyk/snyk/issues/1518#issuecomment-974822508

Screenshot 2021-11-21 at 13 42 53