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

replace download with alternate package #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

itsspriyansh
Copy link
Contributor

No description provided.

@itsspriyansh
Copy link
Contributor Author

itsspriyansh commented May 14, 2024

  1. The person who created the fork (as mentioned in issue Find a replacement for download package #12) has published his code to npm. His package is called @xhmikosr/downloader. We can either download his package or install his fork itself. But here the issue is that we don't have any type declarations for @xhmikosr/downloader. If we rather install his fork directly, the folder structure inside node_modules remains same and only the code gets udpated inside the exact same file as earlier. So in this case maybe the older type declarations might work. However, on doing this, I faced this error in the below image.
    image
    We can have a workaround for this by specifying the type ourselves like this: function(progress: {percent: number}).

  2. In this new patch of the download package, the code has been switched to ES module leading to this error when running the script.
    image
    Can we use dynamic import here to keep commonjs compatibility?

@itsspriyansh itsspriyansh changed the title replace download with alternative package replace download with alternate package May 14, 2024
@garg3133
Copy link
Member

only the code gets udpated inside the exact same file as earlier. So in this case maybe the older type declarations might work.

This is happening because you are using the dev branch instead of the master branch. But we should ideally use the master branch only because otherwise we'll miss out on all the updates that have taken place there. And if after sometime we get another vulnerability, we anyways would need to move to the latest branch.

For the types, we might need to define them ourselves inside the project.

Can we use dynamic import here to keep commonjs compatibility?

Yes, we would need to do that only.

@garg3133
Copy link
Member

You can also look into other similar packages like node-downloader-helper. Since we'd need to do some work to keep the original download package as well, if you find moving to the node-downloader-helper to be easier that this effort, we can consider that as well.

I've used node-downloader-helper package in https://github.com/nightwatchjs/nightwatch-setup-tools to download sample mobile apps when doing the initial setup using the npm init nightwatch command.

@itsspriyansh itsspriyansh force-pushed the download branch 3 times, most recently from e0a663b to 43f08fb Compare May 17, 2024 10:08
@itsspriyansh
Copy link
Contributor Author

itsspriyansh commented May 17, 2024

@garg3133 I have updated the PR. I would like to know if this kind of approach will work. What I have done here is:

  1. Replaced the download package with node-downloader-helper.
  2. I have observed that the node-downloader-helper doesn't come with decompress feature. So I have tried implementing it in our wrapper function itself.
  3. Since the end event of DownloaderHelper triggers immediately after the download is finished and the wrapper function returns at the same time, the files won't be decompressed and the code would break after installing cmdline-tools since we are modifying the folder structure of cmdline-tools immediately after the installation. To handle this, I have used Promise to handle asynchronous operations.
  4. After testing this, the code successfully downloads SDK from scratch without any issue.

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