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

controller-utils/deps: move eth-query to devDeps; remove babel-runtime #1502

Closed
wants to merge 1 commit into from

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Jul 14, 2023

Explanation

While some controllers import eth-query in order to be able to create new EthQuery instances, @metamask/controller-utils only use the package for types.

This moves the package to devDependency and removes babel-runtime, which was imported as an implicit peerDependency of eth-query.

References

Changelog

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@legobeat legobeat added the dependencies Pull requests that update a dependency file label Jul 14, 2023
@Gudahtt
Copy link
Member

Gudahtt commented Jul 14, 2023

The types for this package are currently broken, but we're in the process of fixing them (see #1471)

Unfortunately we do need this in dependencies if the types are referenced in the function signature for any exported methods, which is the case at the moment. If we keep it in devDependencies, it'll break when someone uses this package from a TypeScript project.

@Gudahtt
Copy link
Member

Gudahtt commented Jul 14, 2023

Wait, the constraint is wrong! It's ethjs-query that needs babel-runtime, not eth-query 🤦

See #341

We only use ethjs-query indirectly (via eth-method-registry->ethjs and via nonce-tracker), and only in once controller (the transaction controller). We can remove babel-runtime from everywhere else.

@Gudahtt
Copy link
Member

Gudahtt commented Jul 14, 2023

#1504

@mcmire
Copy link
Contributor

mcmire commented Nov 29, 2023

Is this PR necessary anymore?

@legobeat
Copy link
Contributor Author

Is this PR necessary anymore?

Oh, I had forgotten about this old PR. I guess there needs to be a new take on how to declare type-only import packages before revisiting this, in any case, so closing for now.

@legobeat legobeat closed this Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants