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

Fix cpy vulnerabilities #18471

Closed
wants to merge 5 commits into from
Closed

Conversation

kevcube
Copy link

@kevcube kevcube commented Jun 13, 2022

Issue: cpy 8 has npm audit vulnerabilities

What I did

upgrade to cpy 9.0.1
use flatten: true in places where one file is being copied
remove parents: true because that's the new default behavior

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jun 13, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 845a3e3. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@kevcube kevcube changed the title Fix cpy issues Fix cpy vulnerabilities Jun 13, 2022
@kevcube kevcube mentioned this pull request Jun 13, 2022
3 tasks
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@kevcube thanks for the PR! Unfortunately it's breaking the build since it's pure ESM but being imported in a CJS file.

I think you can get around this via an asynchronous import, something like:

const copy = await import('cpy');
await cpy(defaultFavIcon, options.outputDir);

Can you please give that a try in all the places cpy is used in the code (I believe they are all async context). Thanks!

@kevcube
Copy link
Author

kevcube commented Jun 14, 2022

@shilman fixed the tests! The import statement was correct, which was confusing me. It ended up being something with promise resolution.

@kevcube kevcube requested a review from shilman June 14, 2022 17:41
@kevcube
Copy link
Author

kevcube commented Jun 14, 2022

@shilman I was wrong about fixing the tests, I just got past the one error that was blocking me.

When using async import, typescript gives me this error:

Top-level 'await' expressions are only allowed when the 'module' option is set to 'es2022', 'esnext', 'system', 'node16', or 'nodenext', and the 'target' option is set to 'es2017' or higher.ts(1378)

@shilman
Copy link
Member

shilman commented Jun 14, 2022

@kevcube you can't import it at the top of the file, you should import it inside the async function where it's used. can you give that a try? pretty sure it will work.

@kevcube
Copy link
Author

kevcube commented Jun 14, 2022

@shilman its not working, I've gotten the dynamic import working inside the file, but the tests still aren't passing and I'm not sure why - I've tried a lot of changes with the tsconfig trying to get it to compile to an import rather than a require.. but no dice

@shilman
Copy link
Member

shilman commented Jun 15, 2022

@kevcube OK i'll give it a try and will report back. Thanks!!!

@kevcube
Copy link
Author

kevcube commented Jun 24, 2022

@shilman yeah, the error in the build is the same error I was getting locally. I tried a lot of things to fix but couldn't figure it out. Any ideas?

@shilman
Copy link
Member

shilman commented Jun 27, 2022

Hi @kevcube I think we're going to go with this solution instead, which gets rid of the cpy dependency entirely. Thanks so much for giving it a shot! #18497

@shilman shilman closed this Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants