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

Adding more sources types into ImportFactor.from #52

Open
Siemienik opened this issue Oct 18, 2020 · 4 comments
Open

Adding more sources types into ImportFactor.from #52

Siemienik opened this issue Oct 18, 2020 · 4 comments
Labels
accepted It is accepted to do

Comments

@Siemienik
Copy link
Owner

Siemienik commented Oct 18, 2020

As @Metastasis mentioned in #4 (comment). Current factory has possibility only for reading workbook from local file system, what can't works under browser because security purposes.

Actual

The ImportFactory.from accept only filePath which is a string:

https://github.com/Siemienik/xlsx-import/blob/90c2b76564bd87433de19a9e8f55b9e064d14e09/src/ImporterFactory.ts#L6-L11

Expected

ImportFactory should recognise type of argument and knowing this choose how to handle it.

Type What is Should do
string File path If node: read file from localsystem
string Data url Get ArrayBuffer
string Buffer representation * Transform to ArrayBuffer
Buffer|ArrayBuffer Buffer Read indirectly from buffer
none of above unknown I haven't idea yet, probably should throw an error

I realized that string can also be a buffer so probably my implementation is broken (string can represent buffer as well as file path).

Please keep in mind, that samples Readme link into this issue - should be updated when issue become done.

Further (if requested)

Please do not it now

Type What is Should do
string File url Download from url and read file as ArrayBuffer
@Siemienik Siemienik changed the title Adding more acceptable sources in ImportFactor.from Adding more sources types into ImportFactor.from Oct 18, 2020
@Metastasis
Copy link
Contributor

Metastasis commented Oct 18, 2020

@Siemienik
This issue looks fine but
I'm not sure about File Url part since It looks for me like too much responsibility. From another point of view It could be a lot of copy-paste code in the client therefore this feature should be implemented
I think we could implement this and see what happens next:) (I assume somebody will create an issue if something broken)

@Siemienik
Copy link
Owner Author

Siemienik commented Oct 19, 2020

I'm not sure about File Url part since It looks for me like too much responsibility

Right, I considered a lot about that. May we should implement first version without this and add it if requested. WDYT?

@Metastasis
Copy link
Contributor

@Siemienik
Yeah, I think we could omit File Url

@Siemienik Siemienik added the accepted It is accepted to do label Nov 1, 2020
Siemienik added a commit that referenced this issue Nov 8, 2020
Siemienik added a commit to Metastasis/XToolset that referenced this issue Mar 3, 2021
@Siemienik
Copy link
Owner Author

connected with #196

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted It is accepted to do
Projects
Status: Accepted
Development

No branches or pull requests

2 participants