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

Rewrite XHR code using Axios #1361

Merged
merged 3 commits into from
Apr 25, 2022
Merged

Conversation

lindapaiste
Copy link
Contributor

Remove old and verbose XMLHttpRequest code and replace with axios in:

  • CheckpointLoader
  • CheckpointLoaderPix2pix
  • CVAE

@lindapaiste lindapaiste requested a review from joeyklee April 24, 2022 00:27
Copy link
Contributor

@joeyklee joeyklee left a comment

Choose a reason for hiding this comment

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

Oh yay! Thank you. I am not sure why we didn't use axios since the beginning, but it is SO much more readable. 🥇 !!

@joeyklee joeyklee added the SEMVER/patch a version tag for a patch release change label Apr 25, 2022
@joeyklee joeyklee merged commit a9c5098 into ml5js:main Apr 25, 2022
this.labels = val.labels;
const [modelPathPrefix] = modelPath.split('manifest.json');
axios.get(modelPath).then(({ data }) => {
this.ready = callCallback(this.loadCVAEModel(modelPathPrefix + data.model), callback);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the structure here the same as it was before in terms of "fetch then callCallback". But now that I'm looking over it, the API call really should be moved inside of callCallback to get proper error handling. Probably something for a future PR. I've done a bunch of IIFEs in other places for things like this.

@lindapaiste lindapaiste deleted the fix/xhr-to-axios branch April 25, 2022 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SEMVER/patch a version tag for a patch release change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants