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

Review expensive HTTP operation in archive.html #1187

Open
segun-codes opened this issue Oct 19, 2022 · 5 comments
Open

Review expensive HTTP operation in archive.html #1187

segun-codes opened this issue Oct 19, 2022 · 5 comments

Comments

@segun-codes
Copy link
Collaborator

segun-codes commented Oct 19, 2022

The code snippet below is extracted from /examples/archive.html, I am of the opinion that axios.get(...) shouldn't be invoked without validating that the user-supplied url argument meets certain criteria before attempting to perform the expensive over-the-network get operation albeit there's a failure handling mechanism in place.

I could improve the code by validating the url first, then wrap axios.get(...) codes in an if statement. For instance, if the url must always originate from https://archive.org, then the proposed validation will ensure this. Where validation fails, a small notification message is displayed to the user with the welcomeModal still in view.

@jywaren, what do you think? Can I give this a shot? Many thanks.

form.addEventListener('submit', (event) => {
      event.preventDefault();
      const url = input.value.replace('details', 'metadata');
      axios.get(url)
        .then((response) => {
          if (response.data.files && response.data.files.length != 0) {
            let imageCount = 0;
            response.data.files.forEach(file => {
              if (file.format === 'PNG' || file.format === 'JPEG'){
                let imageRow = document.createElement('div');
                let image = new Image(150, 150);
                let placeButton = document.createElement('a');

                placeButton.classList.add('btn', 'btn-sm', 'btn-outline-secondary', 'place-button');
                placeButton.innerHTML = 'Place on map';

                image.src = `${url.replace('metadata', 'download')}/${file.name}`;
@liliyao2022
Copy link
Contributor

Hi @segun-codes, I think you can also try an async function.

@segun-codes
Copy link
Collaborator Author

segun-codes commented Oct 20, 2022

Okay, thanks @liliyao2022.

@jywarren, @TildaDares still awaiting your comments on this issue. Thank you.

@TildaDares
Copy link
Member

Hi @segun-codes, can you tell us a bit more about how you plan to validate the url? Do you plan to use regex? Thank you!

@segun-codes
Copy link
Collaborator Author

Hi @segun-codes, can you tell us a bit more about how you plan to validate the url? Do you plan to use regex? Thank you!

Hi @TildaDares,

  1. Plan is to use regex to ensure the url meets certain (as your prefer) criteria (e.g., valid url, originates from archive.org, image file extension should be .png | .jpeg | .jpg etc.).
  2. Then codes on line numbers 32 to 89 will simply be wrapped in an if block .
  3. If the url fails validation, then the user is informed via a small modal-based notification message.
  4. Note, at no point here will the welcomeModal become hidden.

What do you think? Your ideas are most welcome too @TildaDares @jywarren. Many thanks.

@segun-codes
Copy link
Collaborator Author

segun-codes commented Oct 22, 2022

Hi @TildaDares, I will proceed to fix and open a PR for this issue, many thanks.

This was referenced Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants