Migrate json rpc engine repo #1804
Conversation
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is new author?A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package. Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
|
I think applying the the core monorepo eslint config and seeing if anything breaks should ideally happen alongside the eslint package version bumps, but as long as it's done before the files are moved into |
Perhaps this can be done in https://github.com/MetaMask/json-rpc-engine prior to actually migrating? What is outstanding after MetaMask/json-rpc-engine#169 ? |
|
Has an assessment be made as how to proceed with MetaMask/json-rpc-engine#81 ? |
@legobeat Yes this can definitely be done in the original repo. I have it as the very first step in the package migration checklist I'm putting together. |
|
I think we should hold on this until 1) we've successfully migrated |
Great question. I know this PR has been sitting around for a while. I will try to make some time tomorrow to think more about this PR. Perhaps it is not as consequential as I imagine. |
@MajorLift The eslint config has already been applied similar to core monorepo, it's here. Just wondering, you see something missing ? |
|
@kanthesha Yes, so aside from aligning the package versions, we also need to align This is achieved by replacing the Here's the relevant PR in the |
* applied eslint rules from core monorepo and fixed the errors * added eslint-import-resolver-typescript devDependencies
* 7.2.0 * release notes modified in changelog --------- Co-authored-by: github-actions <[email protected]> Co-authored-by: Kanthesha Devaramane <[email protected]>
|
Would you be able to clear up the merge commits from main in the commit history here? @mcmire pointed out that they make the history harder to read if you're looking at a tree view of the history. I added this to the checklist but forgot to communicate it to you, sorry about that. You could either do a manual cleanup and force push or just do the For future migrations, we hope to avoid this issue by coordinating with the team to review and close the git history migration PRs ASAP without leaving them open for long. |
|
For context behind @MajorLift's request, we were trying to figure out why the link for the "Unreleased" header for 
But since we've been using merge commits for these library migrations, it has the potential to make the tree view more complex. This complexity makes it more difficult to understand the history and especially locate the branch in the library repo that we are merging with the core monorepo branch. For instance, it creates a view like this. This complexity is created by the merge commits introduced in a previous PR that are now recorded in history: 
Obviously we're not going to be able to get rid of all merge commits. For a library migration branch such as this very one, it's inevitable that as changes are made to Anyway, when you've re-run |
|
Got it, thanks for the detailed explanation @mcmire . |
|
It is available in the new PR. |
Explanation
We would like to migrate most MetaMask libraries into core monorepo to make them easier to maintain.
In this PR, we are migrating json-rpc-engine into core monorepo. In the process of migration, we don't want to remove / loose the original GIT history, but to preserve the history of the library as though the entire project had been located in a merged-packages/json-rpc-engine directory all along.
We chose to place json-rpc-engine initially in the merged-packages and not packages so we can hide it from Yarn, ESLint, Prettier, and TypeScript. This allows us to clean it up and conform it to how we've configured this repo before we properly incorporate it.
References
Fixes https://app.zenhub.com/workspaces/shared-libraries-621e46b4d7103800171d1b02/issues/gh/metamask/core/1550
Changelog
metamask/json-rpc-enginejson-rpc-engineto core monorepoChecklist