Skip to content

Conversation

lucacasonato
Copy link
Contributor

In ESM this is undefined at the top level. Instead to access the global object you must use globalThis.

This is causing some issues in Deno when running WPT idlharness tests, because all code is always executed in a module context.

In ESM `this` is undefined at the top level. Instead to access the global object you must use `globalThis`.
@saschanaz
Copy link
Member

Hmm. Although this should probably be okay, could Deno use the ESM version of webidl2.js inside idlharness instead?

@lucacasonato
Copy link
Contributor Author

As you know the UMD version of webidl2.js is vendored into the wpt repo at https://github.com/web-platform-tests/wpt/blob/master/resources/webidl2/lib/webidl2.js. We could float a patch adjusting how idlharness tests are handled in our wpt fork (https://github.com/denoland/wpt), but this would require significant changes to wpt serve and how idlharness tests are structured. I don't think the overhead is worth it. If this patch doesn't get landed we will likely just float a patch adjusting the this to globalThis in the vendored webidl2.js.

@lucacasonato
Copy link
Contributor Author

For reference, the way we run the tests are to fetch the HTML page from wpt serve, then parse out the script imports, fetch them, and inline them all into a single file together with a custom test reporter. This is the third iteration of the WPT runner, and so far this has yielded the most accurate representation of what WPT test authors expect. Namely that all scripts run inside of the same context.

@saschanaz
Copy link
Member

My concern is that this is a syntactic breaking change, but I guess it should be okay as we expect this to run mainly on latest JS engines.

this would require significant changes to wpt serve and how idlharness tests are structured. I don't think the overhead is worth it.

I hope WPT ultimately migrate to ESM, so I'd say it's kinda worth it 😄

@saschanaz
Copy link
Member

Since there already is a pending breaking change, let's merge this and see how it goes.

@saschanaz saschanaz merged commit 49c514f into w3c:main Apr 27, 2021
@lucacasonato lucacasonato deleted the patch-1 branch April 27, 2021 10:45
lucacasonato added a commit to denoland/wpt that referenced this pull request Jun 5, 2021
foolip pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 7, 2021
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 15, 2021
…estonly

Automatic update from web-platform-tests
chore: update webidl2.js to v24.1.1 (#29238)

Pulls in w3c/webidl2.js#579 for Deno.
--

wpt-commits: 0c0ec487597b6e83f94089ddcef6df854239b10d
wpt-pr: 29238
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Jun 23, 2021
…estonly

Automatic update from web-platform-tests
chore: update webidl2.js to v24.1.1 (#29238)

Pulls in w3c/webidl2.js#579 for Deno.
--

wpt-commits: 0c0ec487597b6e83f94089ddcef6df854239b10d
wpt-pr: 29238
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