fix: properly defer results of a promise

[Wodann]

Fixes the issue raised here by only returning errors when the promise is resolved.

[changeset-bot]

🦋 Changeset detected

Latest commit: 0687ab9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@nomicfoundation/edr	Patch

Not sure what this means? Click here to learn what changesets are.

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 7a298b4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@nomicfoundation/edr	Patch

Not sure what this means? Click here to learn what changesets are.

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

[Copilot]

Pull Request Overview

This PR addresses the issue of deferring promise results by ensuring errors are only returned when the promise is resolved. It updates error handling in the test file, refactors the provider’s callback handling, and improves error propagation in the context creation.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
crates/edr_napi/test/issues.ts	Replaces try/catch error handling with assert.isRejected and renames the variable for clarity
crates/edr_napi/src/provider.rs	Refactors the call_override_callback to use deferred rejection in case of errors
crates/edr_napi/src/context.rs	Introduces early deferred creation and uses match expressions to reject on configuration or build errors
.changeset/fresh-chairs-relate.md	Documents the patch release with a brief description of the fix

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 38 lines in your changes missing coverage. Please review.

Project coverage is 54.21%. Comparing base (6232310) to head (0687ab9).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/edr_napi/src/context.rs	0.00%	16 Missing ⚠️
crates/edr_napi/src/logger.rs	0.00%	16 Missing ⚠️
crates/edr_napi/src/provider.rs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #882      +/-   ##
==========================================
- Coverage   54.23%   54.21%   -0.03%     
==========================================
  Files         252      252              
  Lines       29685    29704      +19     
  Branches    29685    29704      +19     
==========================================
+ Hits        16101    16103       +2     
- Misses      12620    12637      +17     
  Partials      964      964              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[agostbiro]

Thanks, LGTM

[agostbiro]

Hmm on a second look, there are some Hardhat 2 integration tests failing that we should fix before merging I think

[Wodann]

Hmm on a second look, there are some Hardhat 2 integration tests failing that we should fix before merging I think

I'm looking into this. It has to do with my changes, but I'm still uncertain why.

[frangio]

I wanted to see if I could come up with a nicer way to write this since the match statements introduce a lot of noise. We may not want to do this but I'll share what I thought of:

        let (deferred, promise) = env.create_deferred()?;
        let deferred = &std::cell::Cell::new(Some(deferred));

        let try_create_provider = || -> Result<(), napi::Error> {
            let provider_config = edr_napi_core::provider::Config::try_from(provider_config)?;

            let logger_config = logger_config.resolve(&env)?; // <-- keep using ? operator

            // ...

            let deferred = deferred.take().unwrap();

            runtime.clone().spawn_blocking(move || {
                let result = builder.build(runtime.clone()).map(|provider| {
                    Provider::new(
                        provider,
                        runtime,
                        contract_decoder,
                        #[cfg(feature = "scenarios")]
                        scenario_file,
                    )
                });

                deferred.resolve(|_env| result);
            });

            Ok(())
        };

        try_create_provider().unwrap_or_else(|error| {
            deferred.take().unwrap().reject(error);
        });

        Ok(promise)

[Wodann]

We could introduce a macro (for which ? is syntactic sugar) or extract all of the conversions into a local function - resulting in a single match statement. That way we're remaining fully statically typed, as opposed to Cell<Option>'s runtime checks.

[Wodann]

Addressed by 19410ea

[Wodann]

Hmm on a second look, there are some Hardhat 2 integration tests failing that we should fix before merging I think

The issue was resolved in 24b0d94. @agostbiro could you please review this change?

[frangio]

Should we consider using a tokio::oneshot::channel here? Not sure how significant a difference it may be.

[Wodann]

Tokio's version would require async and this is executed in a sync environment.

[Wodann]

I force pushed to create a clean commit message