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

Browser: Add fetch for urls #54

Merged
merged 6 commits into from
Aug 9, 2021

Conversation

DanMHammer
Copy link
Contributor

@DanMHammer DanMHammer commented Jul 27, 2021

Summary

Changes to pdf-merger-js/browser:

  • Adds the ability to pass a url into the merger to be fetched
  • Supports any valid url schema for fetch from an external or internal resource (http, https, about, blob, data, or file). See: fetch documentation
  • Cleans up the code path in _getInputFile a bit to ensure proper execution based on the type of inputFile

Changes to browser-fixtures-test:

  • Adds test for fetching Testfile_A.pdf and Testfile_B.pdf directly from github
  • Adds isomorphic-fetch to devDependencies in order to override fetch for jest testing

Closing Comments

This should allow pdf-merger-js/browser to be more convenient to use. #51 created the browser version to be feature comparable to the non-browser version, but this adds an additional helpful feature that is simple to implement and use due to the native fetch api.

See: #17

Copy link
Collaborator

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

@DanMHammer Cool, another nice improvement! 🎖️

@@ -139,6 +140,28 @@ describe('PDFMerger', () => {
expect(diff).toBeFalsy()
})

test('merge pdfs from urls', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add some Tests to also test ArrayBuffer, Blob, data-url and some invalifd inputs like strings that are no valid urls and maybe even some nonsence-input like numbers and objects, to test the error-path!? Thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Added tests for ArrayBuffer and Buffer.
  • Added a mock fetch function since node-fetch and isomorphic-fetch both cause open handle issues with jest, apparently. All of their documentation and SO threads I could find say to just mock fetch instead.
  • Added tests for mock Blobs
  • Cannot test dataurls or blob urls as those are browser only. jsdom does not currently support simulating them and has no plan to.

@DanMHammer DanMHammer force-pushed the browser/add-fetch-for-urls branch from 73b0e48 to 563bcda Compare August 8, 2021 17:03
@mojoaxel
Copy link
Collaborator

mojoaxel commented Aug 9, 2021

@DanMHammer Thx! 🏅

I contacted @nbesli about giving you access to this project. You are now definitely one of the main contributors! Could you please contact me so that I have an email-adress of you. Thx!

@mojoaxel mojoaxel merged commit 7cc00a5 into nbesli:master Aug 9, 2021
@mojoaxel
Copy link
Collaborator

mojoaxel commented Aug 9, 2021

I consider this a breaking change because the browser "add" function is now async. We need to release this as a major version change. Before doing this I really would like to cleanup the project. It will propably take some more time for this to be released! 🕙


fileReader.readAsArrayBuffer(inputFile)
fileReader.onload = function (evt) {
return fileReader.result
Copy link
Contributor

@Prinzhorn Prinzhorn Sep 10, 2021

Choose a reason for hiding this comment

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

I think this PR broke the FileReader implementation, at least from looking at the code. This return doesn't do anything, because it's an anonymous function. The outer _getInputFile never gets the result and I think it will reach the throw at the bottom?

You can return new Promise(...) that was originally wrapping the whole thing just for this branch

Maybe also reject onerror, currently silently ignored

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.

3 participants