-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(vega-typings): narrow type signature for vega-loader.http #3312
base: main
Are you sure you want to change the base?
Conversation
15578e3
to
debdbf1
Compare
I misspoke - it turns out you can't use fetch with return fetch(url, options).then(response => {
if (response.ok) {
return response.text();
}
return response.text().then(text => {
throw new Error(text);
});
}); |
debdbf1
to
48461c9
Compare
In vega/packages/vega-loader/src/loader.js Line 174 in 774165e
response , which I don't see in RequestInit . Are you sure the type is right?
|
@domoritz, ah, good catch. I referring to this comment from the From logging output at runtime, this is where the options object is populated. The response type would be one of these, or vega/packages/vega-loader/src/formats/index.js Lines 7 to 11 in 02d79dd
Do you think the fix here would be to update the README for vega-loader (text copied below), update the implementation, or both?
|
For questions about the code and readme, @jheer knows best. The typings should follow the implementation. |
Motivation
Checked(You can't use fetch directly, you have to wrap it so that it returns a string. See comment below for a usage example. )vega-loader
, I think it's already behaving as if this type is aResponse
rather than astring
.References
vega/packages/vega-loader/src/loader.js
Lines 175 to 180 in 774165e