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

refactor: use the fetch API #33

Merged
merged 6 commits into from
Oct 15, 2024
Merged

Conversation

lishaduck
Copy link
Contributor

Status

READY

Description

Rewrote it all! :)

In more seriousness:

  1. Uses the fetch API uniformly.
  2. Adds a new use of await and removes a usage of async. Neither should actually affect the behavior.
  3. Moved some code's scope to make it cleaner.

Probably review one-commit-at-a-time?

Closes: #32

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@lishaduck lishaduck requested a review from a team as a code owner September 23, 2024 02:12
@alestiago alestiago changed the title Use the fetch API refactor: use the fetch API Sep 23, 2024
wolfenrain
wolfenrain previously approved these changes Sep 27, 2024
Copy link
Member

@wolfenrain wolfenrain left a 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() {
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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.

@lishaduck
Copy link
Contributor Author

lishaduck commented Sep 27, 2024

Oh, another thing is that this is in sloppy mode, probably worth adding a "use strict" directive.
I don't particularly want to file another PR, but if y'all want to keep this small(er), I don't mind if y'all add it yourselves.

@wolfenrain
Copy link
Member

Oh, another thing is that this is in sloppy mode, probably worth adding a "use strict" directive. I don't particularly want to file another PR, but if y'all want to keep this small(er), I don't mind if y'all add it yourselves.

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.
@tomarra tomarra merged commit 3ed49e5 into VeryGoodOpenSource:main Oct 15, 2024
2 checks passed
@lishaduck lishaduck deleted the fetch branch October 15, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

refactor: use fetch
4 participants