-
Notifications
You must be signed in to change notification settings - Fork 7
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: use the fetch
API
#33
Conversation
Should drop a single microtask. Doesn't really make a difference in practice, but it doesn't hurt either.
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.
LGTM! This is a lot clearer in what it does.
reject(new Error(`Failed to load: ${req.status} ${req.statusText}`)); | ||
} | ||
}; | ||
function reportProgress() { |
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.
This is out of the scope for the refactor but I wonder if we should write some tests for this loader to just verify it works as intended
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.
cc: @alestiago @tomarra
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.
Yes, the initial effort didn't have any tests. I would very much like to have some tests on the template output that verify the loading behavior is as expected; although it might be a bit complex to assess such.
Oh, another thing is that this is in sloppy mode, probably worth adding a |
I am not against adding that here directly, afaik it is just a one-liner right? |
This could theoretically change behavior for `{{flutter_js}}`, but I think it's fine.
Status
READY
Description
Rewrote it all! :)
In more seriousness:
Probably review one-commit-at-a-time?
Closes: #32
Type of Change