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 remix-url-resolver not handling "github:XX" styled package.json packages #5332

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

forefy
Copy link

@forefy forefy commented Oct 25, 2024

Hi 👋

I'm PRing this small change in the remix-url-resolver in cases where the target repo specifies a github: styled repo instead of a regular npm version on it's package.json

Look at forge-std from this example

  "devDependencies": {
    "forge-std": "github:foundry-rs/forge-std#v1.8.1",
    "prettier": "^3.0.0",
    "solhint": "^3.6.2"
  }

This will end up on handleNpmImport, which tries to parse the github:XX string as an NPM package version and errors out

The PR fixes this by just detecting the github prefix, and redirecting execution flow to the correct handleGithubCall handler

To reproduce try to resolve contract repo: https://github.com/PaulRBerg/foundry-template

Please let me know if you have any questions!

Copy link

netlify bot commented Oct 25, 2024

👷 Deploy request for remixproject pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit a511445

@Aniket-Engg
Copy link
Collaborator

@forefy Can you please rebase it?

If the entry is pointing to a github repo, redirect to correct handler instead of continuing
@forefy
Copy link
Author

forefy commented Nov 4, 2024

@forefy Can you please rebase it?

@Aniket-Engg done :)

@Aniket-Engg
Copy link
Collaborator

@forefy which contract should be compiled from shared repo to reproduce the error?

Copy link
Collaborator

@Aniket-Engg Aniket-Engg left a comment

Choose a reason for hiding this comment

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

This still doesn't work as handleGithubCall method performs certain cleaning for URL and resulting URL from github:XX style are not being resolved

@forefy
Copy link
Author

forefy commented Nov 4, 2024

@Aniket-Engg I created a temp PoC repo to demonstrate my point: https://github.com/forefy/remix-url-resolver-poc

To replicate:

  • run npm i
  • run npm run start to see the original error
  • patch in the changes proposed in this PR
  • run npm run start again and see that it works after the change

The difference from what I assumed you tested is how you initialize RemixURLResolver:

const dependencies = {
  deps: {
    "forge-std": "github:foundry-rs/forge-std#v1.8.1", // This was not supported
    prettier: "^3.0.0",
    solhint: "^3.6.2"
  }
};

const resolver = new RemixURLResolver(() => {
  return dependencies;
});

@forefy
Copy link
Author

forefy commented Nov 18, 2024

@Aniket-Engg were you able to replicate the issue now? can we merge this in?

@Aniket-Engg
Copy link
Collaborator

Aniket-Engg commented Nov 21, 2024

@Aniket-Engg I created a temp PoC repo to demonstrate my point: https://github.com/forefy/remix-url-resolver-poc

To replicate:

  • run npm i
  • run npm run start to see the original error
  • patch in the changes proposed in this PR
  • run npm run start again and see that it works after the change

The difference from what I assumed you tested is how you initialize RemixURLResolver:

const dependencies = {
  deps: {
    "forge-std": "github:foundry-rs/forge-std#v1.8.1", // This was not supported
    prettier: "^3.0.0",
    solhint: "^3.6.2"
  }
};

const resolver = new RemixURLResolver(() => {
  return dependencies;
});

POC you prepared is not able to run ts file.
Also, can you add another script with patching the package?

Please consider to add related unit test too in remix-url-resolver package.

@forefy
Copy link
Author

forefy commented Nov 21, 2024

@Aniket-Engg could you please elaborate on what "not able to run ts file" refers to?

Also, it would be simplest IMO to just add the code manually to resolve.js at (see https://github.com/ethereum/remix-project/pull/5332/files#diff-e4cb251c419ec85091200c75e310365042fdfef3a4ffd97cdc1e5c872b903404R177)

For better clarity showing the output you should be expecting and the commands in a more detailed manner:

Before - invalid comparator error on poc example

npm i
npm start

> [email protected] start
> node main.ts

TypeError: Invalid comparator: github:foundry-rs/forge-std#v1.8.1
    at Comparator.parse (/Users/test/node_modules/semver/semver.js:835:11)
    at new Comparator (/Users/test/node_modules/semver/semver.js:818:8)
    at Range.<anonymous> (/Users/test/node_modules/semver/semver.js:1022:12)
    at Array.map (<anonymous>)
    at Range.parseRange (/Users/test/node_modules/semver/semver.js:1021:13)
    at Range.<anonymous> (/Users/test/node_modules/semver/semver.js:965:17)
    at Array.map (<anonymous>)
    at new Range (/Users/test/node_modules/semver/semver.js:964:35)
    at Function.minVersion (/Users/test/node_modules/semver/semver.js:1436:11)
    at RemixURLResolver.<anonymous> (/Users/test/Desktop/remix-url-resolver-poc/node_modules/@remix-project/remix-url-resolver/src/resolve.js:161:72)
Failed to resolve import: Error: Unable to load forge-std/src/Test.sol
    at RemixURLResolver.<anonymous> (/Users/test/Desktop/remix-url-resolver-poc/node_modules/@remix-project/remix-url-resolver/src/resolve.js:187:23)
    at Generator.throw (<anonymous>)
    at rejected (/Users/test/node_modules/tslib/tslib.js:113:69)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

After - working

code node_modules/@remix-project/remix-url-resolver/src/resolve.js
Locate code line, should be 160~170, looks like this

                            if (version) {
                                // PASTE CHANGES HERE
                                const versionSemver = semver_1.default.minVersion(version);
                                url = url.replace(pkg, `${pkg}@${versionSemver.version}`);
                            }

It should end up looking like this:

                            if (version) {
                                // If the entry is pointing to a github repo, redirect to correct handler instead of continuing
                                if (version.startsWith("github:")) {
                                    const [, repo, tag] = version.match(/github:([^#]+)#(.+)/);
                                    const filePath = url.replace(/^[^/]+\//, '');
                                    return this.handleGithubCall(repo, `blob/${tag}/${filePath}`);
                                }
                                const versionSemver = semver_1.default.minVersion(version);
                                url = url.replace(pkg, `${pkg}@${versionSemver.version}`);
                            }

Then you can run npx patch-package @remix-project/remix-url-resolver

And npm run start again to see correct output:

npm run start

> [email protected] start
> node main.ts

Resolved Import: {
  content: '// SPDX-License-Identifier: MIT\n' +
    'pragma solidity >=0.6.2 <0.9.0;\n' +
    '\n' +
    'pragma experimental ABIEncoderV2;\n' +
    ...

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