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

FormData, Response, Request, and Headers have incorrect property descriptors #45099

Open
KhafraDev opened this issue Oct 20, 2022 · 7 comments
Open
Labels
confirmed-bug Issues with confirmed bugs. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@KhafraDev
Copy link
Member

KhafraDev commented Oct 20, 2022

Version

v18.11.0

Platform

Microsoft Windows NT 10.0.19043.0 x64

Subsystem

No response

What steps will reproduce the bug?

The property descriptors should match such that this test succeeds:

for (const global of [
  'Headers',
  'Request',
  'Response',
  'FormData'
]) {
  const desc = Object.getOwnPropertyDescriptor(globalThis, global)
  
  assert.notStrictEqual(desc, undefined)
  assert.deepStrictEqual(desc.value, globalThis[global])
  assert.ok(desc.writable)
  assert.ok(!desc.enumerable)
  assert.ok(desc.configurable)
}

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

the tests from above pass

What do you see instead?

the descriptors are inconsistent with other environments

Additional information

No response

@KhafraDev KhafraDev changed the title FormData, Response, Request, and Headers FormData, Response, Request, and Headers have incorrect property descriptors Oct 20, 2022
@bnoordhuis
Copy link
Member

If you submit that to WPT, then node will fix that next time we update.

If WPT already contains tests but we're not testing that particular submodule, then please see test/wpt/README.md for how to enable them.

@KhafraDev
Copy link
Member Author

The tests exist https://github.com/web-platform-tests/wpt/blob/master/fetch/api/idlharness.any.js

I will look into enabling these tests in node but I've never worked with node's WPT runner and idlharness tests are a little more involved than other tests. 👍

@targos
Copy link
Member

targos commented Oct 22, 2022

I think we already run idlharness tests for other features (for example URL) without issues.

@benjamingr benjamingr added confirmed-bug Issues with confirmed bugs. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Oct 22, 2022
@KhafraDev
Copy link
Member Author

Cool, I'll look into it once a new version of undici is released and is updated in node. Otherwise a few tests will fail that don't anymore.

@aduh95
Copy link
Contributor

aduh95 commented Oct 24, 2022

We should probably make sure that Undici is snapshotted, so we can get rid of the lazy-loading without perf implications.

@techsavvyash
Copy link

Hey Team.
I am a newbie dev starting out in OSS, I would like to take this issue up, can anyone help me navigate through and fix this.
Thanks

@KhafraDev
Copy link
Member Author

I don't think this issue is actionable. Other globals (Blob and File, if File is ever made a global...) were made getters as well in #45659.

Caveat: we have to make some of the globals lazily-loaded too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

No branches or pull requests

6 participants