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

[WIP] add esm support #1994

Closed
wants to merge 1 commit into from
Closed

Conversation

1dcbh
Copy link

@1dcbh 1dcbh commented Oct 26, 2021

WIP! I just created the PR to pull locally, will run tests are report back soon.

@changeset-bot
Copy link

changeset-bot bot commented Oct 26, 2021

⚠️ No Changeset found

Latest commit: 7592f55

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

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

Thanks for sending this PR. Unfortunately, I don't think this approach will work. I left a comment explaining why.

ctx.setHardhatRuntimeEnvironment(env);

env.injectToGlobal();
// TODO: Implication is this becoming async?
Copy link
Member

Choose a reason for hiding this comment

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

This will lead to a race condition. Same for packages/hardhat-core/src/internal/lib/hardhat-lib.ts

@1dcbh
Copy link
Author

1dcbh commented Oct 27, 2021

Yes, sorry for the noise but github.dev required me to create a PR in order to pull this locally

So, my idea is to export an {initialized} field that needs to be awaited by folks that are using es-imports,

import hre from 'hardhat';

async function main () {
  await hre.initialized;
  ...
}

this would be backwards compatible with esm as the try {} catch {} above would not return a promise to them, so it should still keep keep compatibility w/ plugins.

Wdyt?

@alcuadrado
Copy link
Member

Unfortunately that would be a breaking change

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@phated
Copy link

phated commented Nov 18, 2021

The way that hardhat imports tasks and plugins also won't work with ESM because all ESM modules are executed async and hardhat plugins often rely on the ordering of those imports.

@alcuadrado
Copy link
Member

That's an excellent point, @phated! thanks

@fvictorio
Copy link
Member

Everything suggests that adding support for ESM is a major endeavor that we won't be able to do until our next major (v3, which is not even in our roadmap). So I think we should close this, unless you see some way forward @1dcbh.

@alcuadrado alcuadrado closed this Nov 25, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants