Skip to content

Use npm for dependencies#5324

Closed
rickycodes wants to merge 24 commits intomainfrom
use-npm-deps
Closed

Use npm for dependencies#5324
rickycodes wants to merge 24 commits intomainfrom
use-npm-deps

Conversation

@rickycodes
Copy link
Copy Markdown
Contributor

@rickycodes rickycodes commented Nov 30, 2022

previously we were using git for some deps... this was blocking #5147

it's also a security concern in the cases where we weren't pinning to a SHA

I worked with @Gudahtt to fork all the deps and publish them under the @metamask/ namespace.

This PR makes use of the newly published modules.

I'm opening this as a separate PR so #5147 isn't so heavy handed.

  • update git deps to use npm
  • update imports
  • update mocks
  • update patches

@rickycodes rickycodes requested a review from a team as a code owner November 30, 2022 17:48
@rickycodes rickycodes requested a review from sethkfman November 30, 2022 17:57
@rickycodes rickycodes changed the title use npm for dependencies Use npm for dependencies Dec 8, 2022
package.json Outdated
"vm-browserify": "1.1.2",
"zxcvbn": "4.4.2"
"zxcvbn": "4.4.2",
"react-native-tcp": "^3.2.1"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would like to understand how this is getting re-added before we merge this. We don't want two copies of this

Copy link
Copy Markdown
Contributor Author

@rickycodes rickycodes Dec 12, 2022

Choose a reason for hiding this comment

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

after taking a closer look at this one the PR (aprock/react-native-tcp#11/head) had actually been merged. So, I think we're fine to simply use latest (4.0.0)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually wait a minute, it doesn't look like that change is a part of v4.0.0. The changes in that PR diff aren't in the commit tagged as v4, it's not in the commit history either.

Copy link
Copy Markdown
Contributor Author

@rickycodes rickycodes Dec 12, 2022

Choose a reason for hiding this comment

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

ah! nice catch...

idk why I assumed if it was merged it was published 🙃...

I'll continue looking at if we can use the patch approach... alternatively, maybe we can bug the author to publish a new version?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rickycodes

In order to unblock this PR and get the more obvious ones through - could we omit the react-native-tcp change here?

Copy link
Copy Markdown
Contributor Author

@rickycodes rickycodes May 3, 2023

Choose a reason for hiding this comment

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

ya. closing this for now since it's stale (the work) and stale (the context in my brain)

also, apparently the mobile team is in the process of upgrading react native and that will affect all of this... so probably better to wait and do the work interactively as you mentioned

cc @Gudahtt

@rickycodes rickycodes requested a review from Gudahtt December 12, 2022 18:31
@rickycodes rickycodes added the DO-NOT-MERGE Pull requests that should not be merged label Feb 13, 2023
@rickycodes rickycodes closed this May 3, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

DO-NOT-MERGE Pull requests that should not be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants