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

Drag-and-drop Excel #1386

Merged
merged 4 commits into from
Sep 7, 2021
Merged

Drag-and-drop Excel #1386

merged 4 commits into from
Sep 7, 2021

Conversation

joverlee521
Copy link
Contributor

Support drag-and-drop of metadata in Excel format.
Currently only reads in data from the first sheet in the Excel workbook.

@jameshadfield jameshadfield temporarily deployed to auspice-drag-drop-excel-o46qjp August 28, 2021 02:19 Inactive
@jameshadfield jameshadfield temporarily deployed to auspice-drag-drop-excel-o46qjp August 30, 2021 21:16 Inactive
@joverlee521 joverlee521 requested a review from a team August 30, 2021 22:17
Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Thanks Jover, and 💯 for updating the docs contemporaneously.

I tested on a few different CSV, TSV and Excel files (the latter exported by MacOS numbers) and it works well. I was hoping the library would also handle .numbers files, but it doesn't and it's a small user pool compared with .xlsx so not necessary.

See comment above below re: dropping papaparse.

const reader = new FileReader();
reader.onload = async (event) => {
try {
const XLSX = (await import("xlsx/xlsx.mini")).default;
Copy link
Member

Choose a reason for hiding this comment

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

I’ll note here that we’re lazily importing two libraries for drag-and-drop parsing, and each gets bundled separately: xlsx.mini.js (67kb gzipped) and papaparse (7kb gzipped), and this results in two separate browser fetches. Ideally these would be bundled together, but AFAIK this requires them as a cache group to webpack, rather than a JS solution (e.g. Promise.all).

P.S. You can interrogate the bundles via auspice build --analyzeBundle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jameshadfield Would this involve adding a new entry to cacheGroups in webpack.config.js that only contains xlsx and papaparse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+  const dragAndDrop = [
+    "papaparse",
+    "xlsx"
+  ];
+
   /**
    * It's better to keep small libraries out of the vendor bundles because
    * the more libraries we include, the more small changes or updates
@@ -215,6 +222,12 @@ const generateConfig = ({extensionPath, devMode=false, customOutputPath, analyze
             enforce: true,
             chunks: "all"
           },
+          dragAndDrop: {
+            test: new RegExp("[\\\\/]node_modules[\\\\/](" + dragAndDrop.join("|") + ")[\\\\/]"),
+            name: "drag-and-drop",
+            enforce: true,
+            chunks: "all"
+          },

When I add the above to webpack.config.js, I can see both papaparse and xlsx are in one drag-and-drop bundle. When I build and test locally, I see the browser fetches the drag-and-drop bundle but it also fetches a numbered bundle that only contains the following:

(window.webpackJsonp=window.webpackJsonp||[]).push([[13],{751:function(n,w){}}]);

I am very confused why this bundle is created and fetched when it seems to contain nothing
Am I missing something? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I can't answer this without another deep dive into webpack, and given the options available I would advise leaving the config unmodified - i.e. we fetch xlsx and papaparse separately. We could make an issue with the above comments and tag it "revisit sometime".

src/actions/filesDropped/metadata.js Show resolved Hide resolved
Uses the xlsx package to convert dropped files to a CSV string that
can then be parsed by Papa Parse. Currently only reads the first sheet
of Excel files.

We continue to use Papa Parse instead of using `xlsx.utils.sheet_to_json`
because papaparse has built in options for `comments` and `dynamicTyping` that
are not available in xlsx.
@jameshadfield jameshadfield temporarily deployed to auspice-drag-drop-excel-o46qjp September 3, 2021 19:03 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants