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

Adjust GetActiveScriptOrModule to shadow realms #415

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ms2ger
Copy link
Collaborator

@Ms2ger Ms2ger commented Nov 25, 2024

No description provided.

</dl>

<emu-alg>
1. If the execution context stack is empty, return *null*.
Copy link

@shannonbooth shannonbooth Nov 27, 2024

Choose a reason for hiding this comment

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

While this appears to work for HTML integration in the test cases that I have run it against so far (though, I have not tested extensively with importValue) unfortunately I have just found that it breaks some of the test-262 / test-js test cases (previously all test-262 cases were passing).

One of the failing test cases is this test case: https://github.com/LadybirdBrowser/ladybird/blob/5deb2b4cad3755c7e9251f6b23e9b1089b029aa7/Libraries/LibJS/Tests/builtins/ShadowRealm/ShadowRealm.prototype.importValue.js#L22

Where I am now getting:

 FAIL  builtins/ShadowRealm/ShadowRealm.prototype.importValue.js
    ❌ Suite:  normal behavior
         Test:   basic functionality (failed):
                 ExpectationError: Expected target to be null got TypeError: Cannot find/open module: './external-module.mjs'

I will try and investigate a bit more to find out what the problem is. Will also need to cross-check this against tc39/ecma262#3374

Copy link

@shannonbooth shannonbooth Nov 27, 2024

Choose a reason for hiding this comment

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

I will note that it is possible this is also a bug on my end in other areas of module loading outside of this change, but will just mention it incase

edit: I may have been incorrect earlier with saying that tc39/ecma262#3374 fixed the issue I was seeing, there may have been a specific case that it resolved, but on further testing I am running into issues with WPT tests with it. This change resolves the issues I am seeing with WPT, but seems to cause issues in the JS suite I am running against. tc39/ecma262#3374 does not break the JS test suite, but causes issues with WPT test cases. I need to do some further analysis to figure out what is going wrong...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants