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

Dont wait for whole CSV file to get parsed #230

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rajkumarwaghmare
Copy link

Why this change is needed?

  • In case of very large files, waiting time for parsing to finish is considerably long.
  • Instead, just complete the CSV headers to key mapping and return the mapping. This will fasten the process and reduce the waiting time
  • Also, Import Successful message is displayed too early and this wording will confuse user in thinking that the actual CSV file has been mapped, parsed and uploaded/imported. So we need some mechanism where we display Import Successful message only after the data is really "imported" and not just parsed.

Proposed changes

      case StepEnum.MapColumns:
        return (
          <MapColumns
            template={parsedTemplate}
            data={data}
            columnMapping={columnMapping}
            skipHeaderRowSelection={skipHeader}
            selectedHeaderRow={selectedHeaderRow}
            onSuccess={(columnMapping) => {
              setIsSubmitting(true);
              setColumnMapping(columnMapping);
              if (onCSVHeadersMapped) {
                onCSVHeadersMapped(columnMapping).then(() => {
                  setIsSubmitting(false);
                  goNext();
                });
                return;
              }

@rajkumarwaghmare rajkumarwaghmare changed the title Dont wait for whole CVS file to get parsed Dont wait for whole CSV file to get parsed Jun 3, 2024
@ciminelli ciminelli self-requested a review June 20, 2024 18:11
Copy link
Member

@ciminelli ciminelli left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks for the PR. Just one small request on the Promise handling, then let's get this merged.

@@ -1,6 +1,6 @@
{
"name": "csv-import-react",
"version": "1.0.11",
"version": "1.1.2",
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this to 1.0.12? Or I can just increment the version after this is merged.

Copy link
Author

Choose a reason for hiding this comment

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

@ciminelli
Addition of onHeadersMapped is a big update for the library, thats why i incremented the version to 1.1.1 from 1.0.11
And then i fixed a type error that why incremented the version to 1.1.2

Comment on lines +204 to +215
if (onHeadersMapped) {
onHeadersMapped(selectedHeaderRow, columnMapping, originalFile)
.then(() => {
setIsSubmitting(false);
goNext();
})
.catch((error) => {
console.error("onHeadersMapped error", error);
setDataError("An error occurred while processing the data");
});
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm getting this error in testing when onHeadersMapped isn't set: Uncaught TypeError: Cannot read properties of undefined (reading 'then')

Maybe update to use Promise.resolve?

Suggested change
if (onHeadersMapped) {
onHeadersMapped(selectedHeaderRow, columnMapping, originalFile)
.then(() => {
setIsSubmitting(false);
goNext();
})
.catch((error) => {
console.error("onHeadersMapped error", error);
setDataError("An error occurred while processing the data");
});
return;
}
if (onHeadersMapped) {
Promise.resolve(onHeadersMapped(selectedHeaderRow, columnMapping, originalFile))
.then(() => {
setIsSubmitting(false);
goNext();
})
.catch((error) => {
console.error("onHeadersMapped error", error);
setDataError("An error occurred while processing the data");
});
return;
}

Copy link
Author

Choose a reason for hiding this comment

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

@ciminelli
This code block is executed only when onHeadersMapped is defined

if (onHeadersMapped) {
...
}

If you could upload video of how/where you are getting this error, it would be helpful.

Copy link
Member

@ciminelli ciminelli left a comment

Choose a reason for hiding this comment

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

@rajkumarwaghmare I found one more issue. It looks like if you don't include onHeadersMapped, the new condition is still being run and the onComplete doesn't fire. When logging onHeadersMapped, it doesn't come through as undefined but shows:

ƒ () { return fn.apply(this, arguments); }

Can you test by not including onHeadersMapped and see what updates need to be made to have the onComplete still work in that case?

@rajkumarwaghmare
Copy link
Author

@rajkumarwaghmare I found one more issue. It looks like if you don't include onHeadersMapped, the new condition is still being run and the onComplete doesn't fire. When logging onHeadersMapped, it doesn't come through as undefined but shows:

ƒ () { return fn.apply(this, arguments); }

Can you test by not including onHeadersMapped and see what updates need to be made to have the onComplete still work in that case?

I cant reproduce this issue.
Could you please attach the video, that would be helpful.

@ciminelli
Copy link
Member

@rajkumarwaghmare I found one more issue. It looks like if you don't include onHeadersMapped, the new condition is still being run and the onComplete doesn't fire. When logging onHeadersMapped, it doesn't come through as undefined but shows:

ƒ () { return fn.apply(this, arguments); }

Can you test by not including onHeadersMapped and see what updates need to be made to have the onComplete still work in that case?

I cant reproduce this issue. Could you please attach the video, that would be helpful.

I added some log statements to show the issue in the onSuccess handler:

              console.log(onHeadersMapped);
              if (onHeadersMapped) {
                console.log("not running oncomplete");
                Promise.resolve(onHeadersMapped(selectedHeaderRow, columnMapping, originalFile))
                  .then(() => {
                    setIsSubmitting(false);
                    goNext();
                  })
                  .catch((error) => {
                    console.error("onHeadersMapped error", error);
                    setDataError("An error occurred while processing the data");
                  });
                return;
              }
              console.log("running oncomplete");

Here is what happens (with onHeadersMapped not being passed in):

Screen.Recording.2024-06-25.at.13.50.46.mov

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.

2 participants