-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
dedup Headers types #4736
dedup Headers types #4736
Conversation
errMsg, | ||
`Headers.${method} requires at least 1 argument, but only 0 present` | ||
`${method} requires at least 1 argument, but only 0 present` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kitsonk ... with this change we have this unfortunate side-effect:
> ./target/debug/deno
> Headers.name
HeadersImpl
I'm sure we can workaround this somehow - but it's not immediately obvious to me how.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the case of HeadersImpl, it could be done in dom_iterable.ts
deno/cli/js/web/dom_iterable.ts
Lines 82 to 86 in 8397cd5
// we want the Base class name to be the name of the class. | |
Object.defineProperty(DomIterable, "name", { | |
value: Base.name, | |
configurable: true, | |
}); |
however there are other classes, like URLSearchParams which would also have to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were it is meaningful, we just need to override it. I wouldn't put it in dom_iterable
because that overloads what it does it... just something like the below and throw it in util somewhere:
export function setName(fn: Function, value: string): void {
Object.defineProperty(fn, "name", { value, configurable: true };
}
You should be able to modify the class after you export it as well... so no need to assign it to any variable or anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would work... it is more overhead than just a normal class definition (not much, admittedly, and maybe none after snapshotting). Ultimately we're just working around our own (dis)organization - ideally the JS that gets executed would just be
class Header {
// ...
}
not
class HeaderImpl {
// ..
}
Object.defineProperty(HeaderImpl, "name", { "Header", configurable: true });
I don't have a good solution on how to get our TS to produce the optimal JS :/
No description provided.