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

Add `onDownloadProgress` option #34

Merged
merged 43 commits into from May 10, 2019

Conversation

Projects
None yet
@dangdennis
Copy link
Contributor

commented Sep 22, 2018

Here's the new PR for the potential stream API.

Basic example for reading an image stream:

function streamProgressCallback(percentage, bytes) {
     console.log('percentage:: ' + percentage)
     console.log('bytes:: ' + bytes)
}

const res = await ky.get(someImageURL, { stream: true, streamProgressCallback }).blob();

const imgObject = URL.createObjectURL(res);
const img = document.createElement('img');
img.src = img1;
document.body.appendChild(img)

Not sure how to test streams.


Fixes #3
Fixes #6

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Sep 26, 2018

I don't think node-fetch supports Stream, so we need to test in a real browser.

What's the best tool for browser testing these days? Preferably something using Puppeteer. Probably https://github.com/direct-adv-interfaces/mocha-headless-chrome or https://github.com/mantoni/mochify.js // @sholladay

@sholladay

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2018

I am aware of three high quality solutions for browser testing:

  • Puppeteer, which you already mentioned. My team recently used this to debug problems with our signup system. Turns out we had a cookie with an expiration time that was off by a few zeros (seconds vs milliseconds), which meant you had to fill out a form relatively quickly or it would fail. Puppeteer was very helpful to whip up a quick script to reliably reproduce the issue and narrow it down to a timing issue and eventually identify the root cause. I liked it, it was nice. I would not use it full time, though, because it's proprietary and only works with Chrome.
  • Cypress, which is up and coming. They are trying to get around some of the limitations of WebDriver and to improve performance. It also does a cool thing where it records the state history so you can go backwards in time, a la the Redux DevTools. Unfortunately, it also only works with Chromium based browsers at the moment. They supposedly have plans to support other browsers, but I doubt it will happen any time soon (very hard problem to solve correctly).
  • Intern, which is what I use at companies. It wraps WebDriver in an easy to use package and handles a lot of bugs and edge cases automatically. It is the only one that is based on open web standards and is completely cross-browser. It is configuration-centric with good defaults. It has a great community and team behind it, along with a solid feature set (I paid SitePen in the past to develop some of them). At times it feels a bit verbose, but overall it is my favorite.

SitePen wrote a post about Puppeteer that is worth reading.

For ky, a lot of it will depend on your goals. Do you want to be able to test in Firefox, Safari, or Edge? If so, I would go with Intern. Otherwise, I think Cypress is the most appropriate (it was designed with CI in mind). But there are still lots of tools in the Puppeteer ecosystem I have not tried yet, so maybe there is some awesome test runner integration out there. I see some ava based boilerplates (e.g. ava-puppeteer and ava-puppeteer-example, but I am not sure how well they work, though.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Sep 29, 2018

Sounds like Cypress would be the best fit then. I only care about testing it with Chromium.

@dangdennis Would you be up for trying to integrate some Cypress tests?

@dangdennis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

Thanks for the extensive thoughts @sholladay.

I'll take this - I'll set up Cypress and a minimal test process.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Sep 30, 2018

I'll take this - I'll set up Cypress and a minimal test process.

Awesome! Can you do it as a separate PR? Since there are other PRs that need it too, so it's not blocked by this PR.

@ianwalter

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

I'm a fan and use Cypress every day but I think it is overkill and not super well-suited for this.

For unit tests like these, I would recommend Karmatic which wraps Puppeteer, Karma, and Jasmine.

@sholladay

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2018

Hmm I've used Karma but never heard of Karmatic. It's emblematic of the problem that Karma has, though... it needs a layer on top of it to be manageable and useful. I do like the developer experience of a full Karma environment when it's properly set up, but this seems like it's just hiding the complexity.

Some specific problems I see:

  • Karmatic downloads an entire Chromium installation. It's enormous! The install size is 273MB. This will slow down CI runs and ties us down to whatever version it feels like downloading. Any decent CI system comes with a built-in browser, which is usually pre-configured with certain settings for debugging, and it would be better to use that, both for performance reasons and because we're in control of it.
  • Karmatic has a peer dependency on Webpack, which I highly doubt Sindre will want to add to Ky. But we'll see.
@dangdennis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

Sorry for the delay folks. This PR is blocked by the other Cypress PR to fulfill. That's being delayed by a bug within Cypress and Typescript (or something, I forget).

Once that PR is merged, we can continue working on a method for end-to-end testing for the stream API.

@ianwalter

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

@sholladay If you think that is big 😄
image

But I am thankful that Cypress now uses a global cache.

@sholladay

This comment has been minimized.

Copy link
Collaborator

commented Oct 23, 2018

The Cypress install cache helps a lot, but I don't remember it ever taking that long to download. I will look into that more later. For now, here's a quick comparison that shows the caching in action. I am on slow Wi-Fi right now, so these numbers are rather extreme and should be taken with a grain of salt. But it's nonetheless a big difference.

Cypress

$ time npm install --save-dev cypress

> cypress@3.1.0 postinstall /private/tmp/foo/node_modules/cypress
> node index.js --exec install

Cypress 3.1.0 is already installed in /Users/sholladay/Library/Caches/Cypress/3.1.0

+ cypress@3.1.0
added 197 packages from 168 contributors and audited 327 packages in 4.615s
found 0 vulnerabilities

        4.94 real        10.02 user        12.54 sys
$ du -sh node_modules
 51M    node_modules

Karmatic

$ time npm install --save-dev karmatic
npm WARN deprecated karma-webpack@2.0.7: Deprecated due to publishing issue, use 2.0.8
npm WARN deprecated nodemailer@2.7.2: All versions below 4.0.1 of Nodemailer are deprecated. See https://nodemailer.com/status/
npm WARN deprecated node-uuid@1.4.8: Use uuid module instead
npm WARN deprecated hoek@2.16.3: The major version is no longer supported. Please update to 4.x or newer
npm WARN deprecated socks@1.1.9: If using 2.x branch, please upgrade to at least 2.1.6 to avoid a serious bug with socket data flow and an import issue introduced in 2.1.0
npm WARN deprecated mailcomposer@4.0.1: This project is unmaintained
npm WARN deprecated buildmail@4.0.1: This project is unmaintained
npm WARN deprecated uws@9.14.0: stop using this version

> fsevents@1.2.4 install /private/tmp/foo/node_modules/fsevents
> node install

[fsevents] Success: "/private/tmp/foo/node_modules/fsevents/lib/binding/Release/node-v57-darwin-x64/fse.node" already installed
Pass --update-binary to reinstall or --build-from-source to recompile

> uws@9.14.0 install /private/tmp/foo/node_modules/uws
> node-gyp rebuild > build_log.txt 2>&1 || exit 0


> puppeteer@1.9.0 install /private/tmp/foo/node_modules/puppeteer
> node install.js

Downloading Chromium r594312 - 82.7 Mb [====================] 100% 0.0s
Chromium downloaded to /private/tmp/foo/node_modules/puppeteer/.local-chromium/mac-594312

> circular-json@0.5.8 postinstall /private/tmp/foo/node_modules/circular-json
> echo ''; echo -e "\x1B[1mCircularJSON\x1B[0m is in \x1B[4mmaintenance only\x1B[0m, \x1B[1mflatted\x1B[0m is its successor."; echo ''


-e CircularJSON is in maintenance only, flatted is its successor.

npm WARN karmatic@1.2.0 requires a peer of webpack@>=4 but none is installed. You must install peer dependencies yourself.
npm WARN babel-loader@7.1.5 requires a peer of webpack@2 || 3 || 4 but none is installed. You must install peer dependencies yourself.
npm WARN karma-webpack@2.0.7 requires a peer of webpack@^1.0.0 || ^2.0.0 || ^3.0.0 but none is installed. You must install peer dependencies yourself.
npm WARN webpack-dev-middleware@1.12.2 requires a peer of webpack@^1.0.0 || ^2.0.0 || ^3.0.0 but none is installed. You must install peer dependencies yourself.

+ karmatic@1.2.0
added 668 packages from 729 contributors and audited 5735 packages in 386.563s
found 7 vulnerabilities (2 low, 5 moderate)
  run `npm audit fix` to fix them, or `npm audit` for details
      386.89 real        29.45 user        27.16 sys
$ du -sh node_modules
265M    node_modules

4.94 seconds vs 386.89 seconds (almost 6.5 minutes).

Interestingly, Karmatic is 8MB smaller today. But on the other hand, look at all of those warnings and security vulnerabilities... leaves me with the impression they don't care about the dependency tree.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Oct 27, 2018

@sholladay Cypress is not working though (#51) and it doesn't seem like they plan to do a new release anytime soon (last release was in August), so I would rather go for something that works now (Karmatic) than something that might work better in the future (Cypress).

@dangdennis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2018

Sure, that makes sense enough.

Anyone want to make that Karmatic PR? Then I'll add the stream test within here.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Nov 1, 2018

We're now using Cypress, so this PR can be continued.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Nov 1, 2018

@dangdennis I don't think we even need a stream option yet. Let's go with the minimal thing to expose, a onProgress option.

const onProgress = stats => {
	console.log(stats.percent, stats.bytesRead, stats.bytesTotal);
};

const res = await ky(someImageURL, {onProgress}).blob();

We need to make sure it still works fine with the retry feature.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Nov 1, 2018

Can you also fix the merge conflict and rebase from master branch?

@sindresorhus sindresorhus force-pushed the sindresorhus:master branch from 383d754 to f107e7b Nov 1, 2018

@dangdennis dangdennis force-pushed the dangdennis:stream branch from 7a87fc2 to e75b4c3 Nov 1, 2018

@dangdennis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

If onProgress is passed, Ky will automatically stream the response. Unfortunately, @sindresorhus I still can't get the build to pass.

 ✖ No tests found in test/hooks.js
  ✖ No tests found in test/main.js
  ✖ No tests found in test/prefix-url.js
  ✖ No tests found in test/retry.js

  4 uncaught exceptions

  Uncaught exception in test/main.js

  /Users/dennis/Desktop/ky/file:/test/_require.js:7

  file:///Users/dennis/Desktop/ky/test/_require.js:1



  Uncaught exception in test/hooks.js

  /Users/dennis/Desktop/ky/file:/test/_require.js:7

  file:///Users/dennis/Desktop/ky/test/_require.js:1



  Uncaught exception in test/prefix-url.js

  /Users/dennis/Desktop/ky/file:/test/_require.js:7

  file:///Users/dennis/Desktop/ky/test/_require.js:1



  Uncaught exception in test/retry.js

  /Users/dennis/Desktop/ky/file:/test/_require.js:7

  file:///Users/dennis/Desktop/ky/test/_require.js:1

Also, I just fought with Git for an hour trying to squash my commits. So glad that's over with. However, I still haven't figured out how to fix merge conflicts and rebase without a commit.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Nov 1, 2018

Unfortunately, @sindresorhus I still can't get the build to pass.

Probably because of https://github.com/sindresorhus/ky/pull/34/files#diff-cd87bb3712c5cc8ef9426451d8b99768L2

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Nov 1, 2018

Also look at the diff, there are still many leftovers from the incorrect merge conflict resolving that needs to be fixed, like https://github.com/sindresorhus/ky/pull/34/files#diff-0730bb7c2e8f9ea2438b52e419dd86c9L121

@dangdennis dangdennis force-pushed the dangdennis:stream branch 7 times, most recently from a841491 to ab8858a Nov 1, 2018

@sholladay

This comment has been minimized.

Copy link
Collaborator

commented Nov 1, 2018

I'm willing to help on this. If I can get write access to the fork, I'll work on tests and see what I can do about the diff and commit history.

@dangdennis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

@sholladay That'd be helpful. Is that something @sindresorhus can enable?

szmarczak added some commits Apr 22, 2019

@szmarczak szmarczak requested a review from sindresorhus Apr 22, 2019

@szmarczak

This comment has been minimized.

Copy link
Collaborator

commented Apr 22, 2019

I'll have to make a workaround to check undefined chunk.

const puppeteer = require('puppeteer');

(async () => {
    const browser = await puppeteer.launch();
    const page = await browser.newPage();

    await page.goto('about:blank');

    const result = await page.evaluate(async url => {
        return [undefined];
    }, 'about:blank');

    console.log(result);

     await page.close();
     await browser.close();
})();
$ nano demo.js
[ null ]

szmarczak added some commits Apr 22, 2019

@dangdennis

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

@szmarczak Thank you for taking this to completion - 6 month PR!

@szmarczak

This comment has been minimized.

Copy link
Collaborator

commented Apr 22, 2019

Yes, it was sent 6 months ago. But it needed just a few days to complete :P

@szmarczak szmarczak changed the title Add `onProgress` option Add `onDownloadProgress` option Apr 23, 2019

@szmarczak szmarczak requested a review from sholladay Apr 23, 2019

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2019

@szmarczak #34 (comment) is not done.

Show resolved Hide resolved test/browser.js Outdated
Show resolved Hide resolved test/browser.js Outdated
@szmarczak

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

@szmarczak #34 (comment) is not done.

Oh, forgot about that. Will change.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented May 9, 2019

@szmarczak You also need to fix the merge conflict.

szmarczak added some commits May 10, 2019

Show resolved Hide resolved index.d.ts Outdated
Show resolved Hide resolved index.d.ts Outdated
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved readme.md Outdated
Show resolved Hide resolved readme.md Outdated
Show resolved Hide resolved readme.md Outdated
Show resolved Hide resolved readme.md Outdated

@szmarczak szmarczak requested a review from sindresorhus May 10, 2019

sindresorhus added some commits May 10, 2019

@sindresorhus sindresorhus merged commit f89d796 into sindresorhus:master May 10, 2019

4 checks passed

Travis CI - Pull Request Build Passed
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 7793c53
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sindresorhus

This comment has been minimized.

Copy link
Owner

commented May 10, 2019

Thanks all! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.