-
Notifications
You must be signed in to change notification settings - Fork 202
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
refactor response create function outside of promise handler #132
base: main
Are you sure you want to change the base?
Conversation
This will depend on the size impact - I've updated the repo to include a size check, we'll see what it nets. |
Ah - it looks like this adds a named export to the actual unfetch package, which means it won't work - folks using CommonJS will have to do Instead, if we could retrofit your work here into proper support for the Response interface, that would definitely be a justifiable export! It would be a nice step towards supporting |
|
@kalisjoshua that'll be the added export. One quick solution to check this would be to move the |
not sure that this is favorable or not but thought that it might be and it allows for easier testing of the creation of responses and their properties.
1477655
to
ac63b91
Compare
This is done now.
I will look into the Response interface compatibility. |
|
Interesting that the numbers are still so increased. My guess is that's because we're ending up with a closure around the two top-level functions, whereas currently there is no closure required since all functionality exists within the single |
It's interesting to look at the compiled output diff of |
I think the biggest contributor to the bloat that I created is because of the lifting of the |
673f183
to
3986993
Compare
|
not sure that this is favorable or not but thought that it might be and it allows
for easier testing of the creation of responses and their properties.