FEATURE: get rid of the import a query modal (PR #127)

This is the new version of FEATURE: open a system file picker directly instead of opening the import modal by AndrewPrigorshnev · Pull Request #126 · discourse/discourse-data-explorer · GitHub. Now the pick-files-button moved to core.

To import a query into Data Explorer, you need to push the button and then deal with the import modal:

124735139-d168f500-df26-11eb-8b71-82f0ada18129

124735161-d75ed600-df26-11eb-943b-024c873df6da

Instead, we want just to be triggering a system file picker directly. This PR makes it happen.

GitHub

The title of this pull request changed from “Feature/get rid of the import a query modal 2” to "FEATURE: get rid of the import a query modal

I am just wondering why we need all these variants of the JSON type? Do different browsers send different things? Usually it is application/json

Looking good and I like the new component, just a few questions.

I think we should add some JSON parse error handling here, in case the JSON in the file is not correctly structured. Then you can show a message to the user and reset the loading state

Probably want to implement FileReader.onerror here as well to be able to reset the state and show a message on error. If this is a pattern we use in a few places in future we may want to refactor this out into a lib that we can import. No need for that yet though as this is the only place we are doing this so far :slight_smile:

E.g. app/lib/file-reader.js

You will need to set loading to false here too I assume?

Also do we know what the structure should look like ahead of time? Maybe we should check for the keys that are required and throw an error if the user doesn’t provide them?

I don’t know initial intentions behind this list, I’ve just moved it from other place in this PR. But looks like you’re right and it’s more than enough to have just application/json. For example, text/javascript is obsolete, and application/x-javascript was experimental .

All comments about error handling and setting loading to false are slightly out of the scope of this PR, I wanted just to get rid of an unnecessary modal and carefully move existing code.

At the same time, they all are perfect catches, and we definitely need to fix them. So I fixed these problems and I also switched these methods to async/await pattern, all these checks implemented with promise chaining would look overcomplicated.

Speaking of extracting code for reading files to lib, this wrapper that can be awaited can be our starting point:

  _readFileAsTextAsync(file) {
    return new Promise((resolve, reject) => {
      const reader = new FileReader();
      reader.onload = () => {
        resolve(reader.result);
      };
      reader.onerror = reject;

      reader.readAsText(file);
    });
  }

And the mime type for json text is application/json.

We also already use just application/json with file inputs in several places, it seems to work fine.

Shouldn’t this function be marked async if you are awaiting it in _importQuery?

Much better, still a few minor changes though.

I don’t think this function needs to be async, nothing that is promise-related is happening here.

Much better, thanks for adding the error handling

Nitpick, should be "which should at least have an 'sql' property". Also…I don’t mind if you change this or not, it’s just another nitpick, but I think JSON should be capitalised because it is an acronym

Thanks for changing this. I think the other modal was done in 2014 so maybe this wasn’t the accepted mime type back then? Anyway application/json is totally fine, never used anything but that.

Oh, this is redundant here, a good catch!

No, we don’t need it here. Normally, you need async if you’re awaiting something inside the function. This function just create and return a promise, and we’re awaiting that promise outside.