Jamie5 Github contribution chart
Jamie5 Github Stats
Jamie5 Most Used Languages

Activity

23 Aug 2022

Issue Comment

Jamie5

Issue when aborting a transaction in progress

Is this issue a feature request or a bug report?

Bug report

What is the current behavior?

  1. xhr.send gets called
  2. In send, the following happens: setTimeout(() => onSend.call(this, this), 0)
  3. Meanwhile, the code will call xhr.abort()
  4. Now, onSend gets called and by default goes to _handleRequest, which will eventually call request handlers even though the request was already aborted. The request handlers might e.g. check the url, which has been deleted by _terminateRequest.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

What is the expected behavior?

Since the request was already aborted, we shouldn't be hitting the handlers or anything.

Environment in which you encounter the issue:

  • mock-xmlhttprequest version: 7.0.4
  • Node.js version: N/A
  • OS version:

Did this work in a previous version of mock-xmlhttprequest (which)?

Do you intend to provide a pull request to address this issue?

Forked On 08 Aug 2022 at 07:10:46

Jamie5

Ran into fun webpack issues while trying to see, but based on the code it looks very promising! Thank you!

Commented On 08 Aug 2022 at 07:10:46

Jamie5

Add the ability to screenshot a part of the screen (#3)

Pushed On 22 Jul 2022 at 12:07:54
Pull Request

Jamie5

Add the ability to screenshot a part of the screen

Created On 22 Jul 2022 at 12:07:53
Issue Comment

Jamie5

Feature Request: Take screenshot of a portion of the page

Thanks for making such a great library available for everyone to use. At our company we're getting a lot of use out of it :)

One feature we need internally is to take a screenshot of a portion of particular stories. Specifically, we render out dom nodes with a data-canvas-strip attribute and we're interested in ensuring that the screenshot only contains that node and nothing else. We don't necessarily know the height of each story ahead of time (it varies between stories and hardcoding is isn't portable across different viewports) so simply cropping the screenshots after taking them isn't feasible.

I've attached a proof of concept diff which adds a selector field which takes a CSS selector to the ScreenshotOptions interface and it works great for us, but we'd like to upstream it if possible so that we don't need to maintain a fork and so that other people can use the feature if they think it's useful.

I'm more than happy to take the diff and improve it so that it can be PRed. I would just need some pointers on the following:

  • Is my general approach correct? I've added a captureRawScreenshotBuffer method to the CapturingBrowser class which handles the selector option. This was the first place I saw I could add it, so I'm not sure if there's a more appropriate location for it.
  • If the user provides a selector that doesn't match an element, should that be an error or should we just fall back to capturing the entire viewport? We don't hit this case in our code because we always have a valid selector but of course it could happen to someone using the package.
  • Is there any need for test cases? The only test I can see is the e2e.sh file which runs storycap and asserts that at least one screenshot file is emitted. If that's the only test currently then I assume this feature doesn't need any additional tests?

Please let me know your thoughts on this and I'll get to work on a proper PR if you're happy to accept one for this feature.


Current patch:

diff --git a/packages/storycap/src/node/capturing-browser.ts b/packages/storycap/src/node/capturing-browser.ts
index 64c49ee..ffea89a 100644
--- a/packages/storycap/src/node/capturing-browser.ts
+++ b/packages/storycap/src/node/capturing-browser.ts
@@ -307,6 +307,28 @@ export class CapturingBrowser extends StoryPreviewBrowser {
     }
   }
 
+  private async captureRawScreenshotBuffer(screenshotOptions: ScreenshotOptions) {
+    if (screenshotOptions.selector) {
+      await this.page.waitForSelector(screenshotOptions.selector);
+      const element = await this.page.$(screenshotOptions.selector);
+
+      if (!element) {
+        // TODO: What to do?
+        throw new Error();
+      }
+
+      return element.screenshot({
+        fullPage: screenshotOptions.fullPage,
+        omitBackground: screenshotOptions.omitBackground,
+      });
+    } else {
+      return this.page.screenshot({
+        fullPage: screenshotOptions.fullPage,
+        omitBackground: screenshotOptions.omitBackground,
+      });
+    }
+  }
+
   private logInvalidVariantKeysReason(reason: InvalidVariantKeysReason | null) {
     if (reason) {
       if (reason.type === 'notFound') {
@@ -399,10 +421,7 @@ export class CapturingBrowser extends StoryPreviewBrowser {
     await this.page.evaluate(() => new Promise(res => (window as any).requestIdleCallback(res, { timeout: 3000 })));
 
     // Get PNG image buffer
-    const rawBuffer = await this.page.screenshot({
-      fullPage: emittedScreenshotOptions.fullPage,
-      omitBackground: emittedScreenshotOptions.omitBackground,
-    });
+    const rawBuffer = await this.captureRawScreenshotBuffer(emittedScreenshotOptions);
 
     let buffer: Buffer | null = null;
     if (Buffer.isBuffer(rawBuffer)) {
diff --git a/packages/storycap/src/shared/screenshot-options-helper.ts b/packages/storycap/src/shared/screenshot-options-helper.ts
index 3868b0a..c2d4e6e 100644
--- a/packages/storycap/src/shared/screenshot-options-helper.ts
+++ b/packages/storycap/src/shared/screenshot-options-helper.ts
@@ -69,6 +69,7 @@ export function createBaseScreenshotOptions({
       viewport: viewports[0],
       variants: viewports.slice(1).reduce((acc, vp) => ({ ...acc, [vp]: { viewport: vp } }), {}),
       defaultVariantSuffix: viewports[0],
+      selector: '',
     };
   } else {
     return {
@@ -77,6 +78,7 @@ export function createBaseScreenshotOptions({
       waitAssets: !disableWaitAssets,
       viewport: viewports[0],
       defaultVariantSuffix: '',
+      selector: '',
     };
   }
 }
diff --git a/packages/storycap/src/shared/types.ts b/packages/storycap/src/shared/types.ts
index cd15abe..1690b9a 100644
--- a/packages/storycap/src/shared/types.ts
+++ b/packages/storycap/src/shared/types.ts
@@ -23,6 +23,7 @@ export interface ScreenshotOptionFragments {
   click?: string;
   skip?: boolean;
   omitBackground?: boolean;
+  selector?: string;
 }
 
 export interface ScreenshotOptionFragmentsForVariant extends ScreenshotOptionFragments { 

Forked On 21 Jul 2022 at 12:56:05

Jamie5

https://github.com/remix/storycap/pull/3/files is a similar way to do things, in which we manually specify the bounding box (which may have its uses, especially when the actual screenshot-taking code is wrapped in one's own layer anyways)

Commented On 21 Jul 2022 at 12:56:05
Pull Request

Jamie5

Add the ability to screenshot a part of the screen

Created On 21 Jul 2022 at 12:43:55
Create Branch
Jamie5 In Jamie5/storycap Create Branchjamie/target

Jamie5

A Storybook Addon, Save the screenshot image of your stories :camera: via puppeteer.

On 21 Jul 2022 at 12:43:31
Pull Request

Jamie5

Test

Created On 21 Jul 2022 at 12:00:15
Pull Request

Jamie5

Test

Created On 20 Jul 2022 at 11:23:20
Create Branch
Jamie5 In Jamie5/storycap Create Brancholdwork

Jamie5

A Storybook Addon, Save the screenshot image of your stories :camera: via puppeteer.

On 20 Jul 2022 at 11:07:03

Jamie5

Edit package.json for new org (#2)

Pushed On 20 Jul 2022 at 09:54:59

Jamie5

Edit package.json for new org (#2)

Pushed On 20 Jul 2022 at 09:49:22
Pull Request

Jamie5

Edit package.json for new org

Created On 20 Jul 2022 at 09:49:22
Pull Request

Jamie5

Edit package.json for new org

Created On 20 Jul 2022 at 09:01:00
Create Branch
Jamie5 In Jamie5/storycap Create Branchjamie/pacakge

Jamie5

A Storybook Addon, Save the screenshot image of your stories :camera: via puppeteer.

On 20 Jul 2022 at 09:00:34

Jamie5

x

Pushed On 20 Jul 2022 at 08:59:59

Jamie5

Expose setViewport function

Currently, storycap, when taking screenshots, does not resize the viewport until the point of screenshotting. Hence the initial viewport might be different, which can cause issues. For example, if the viewport is too small, responsive design might hide some text which we are then unable to find. The desired solution is to set the viewport before anything has happened. Hence, we expose the setViewport method to do so.

Pushed On 20 Jul 2022 at 08:59:59

Jamie5

x

Pushed On 20 Jul 2022 at 04:45:28

Jamie5

Expose setViewport function

Currently, storycap, when taking screenshots, does not resize the viewport until the point of screenshotting. Hence the initial viewport might be different, which can cause issues. For example, if the viewport is too small, responsive design might hide some text which we are then unable to find. The desired solution is to set the viewport before anything has happened. Hence, we expose the setViewport method to do so.

Pushed On 20 Jul 2022 at 04:45:28
Pull Request

Jamie5

Expose setViewport function

Created On 20 Jul 2022 at 04:45:27
Pull Request

Jamie5

WIP

Created On 19 Jul 2022 at 06:03:51
Create Branch
Jamie5 In Jamie5/storycap Create Branchjamie/viewport

Jamie5

A Storybook Addon, Save the screenshot image of your stories :camera: via puppeteer.

On 19 Jul 2022 at 06:03:41

Jamie5

A Storybook Addon, Save the screenshot image of your stories :camera: via puppeteer.

Forked On 19 Jul 2022 at 06:02:00
Issue Comment

Jamie5

Expose setViewport on window

The request is to, in https://github.com/reg-viz/storycap/blob/master/packages/storycap/src/node/capturing-browser.ts , add to expose() another function, something like setViewport: (vp) => this.page.setViewport(vp), so that a test could do await window.setViewport(...)

Why?

We now have play functions, and hence UI interactions. These UI interactions may differ due to the size of the UI (i.e. responsive design might hide text when the window is narrow), which fundamentally goes to the window size. So it would be nice to be able to set the window size at the beginning of the test, rather than only when the screenshot happens. Otherwise play functions might become flaky and confusing.

Note that any other solution to the problem would be fine too - only that it would be strongly preferred for it to be possible to resize the viewport dynamically, rather than e.g. declaring in a test's parameters that it should have a given set of dimensions.

It might be possible to pass arguments to storycap to have a given viewport, but such a global option would not be preferred as individual tests might have individual viewports.

Forked On 14 Jul 2022 at 04:59:50

Jamie5

https://github.com/Jamie5/storycap/pull/1/commits/cff4d388903cebc908d2ebe2e3f7ba156af707ba works, though I imagine you would prefer to not just expose things on window itself. If this general feature is something you'd like to have I can work on something that is more wrapped properly, as it would be nice not to have to maintain forks and stuff.

Commented On 14 Jul 2022 at 04:59:50
Issue Comment

Jamie5

Issue when aborting a transaction in progress

Is this issue a feature request or a bug report?

Bug report

What is the current behavior?

  1. xhr.send gets called
  2. In send, the following happens: setTimeout(() => onSend.call(this, this), 0)
  3. Meanwhile, the code will call xhr.abort()
  4. Now, onSend gets called and by default goes to _handleRequest, which will eventually call request handlers even though the request was already aborted. The request handlers might e.g. check the url, which has been deleted by _terminateRequest.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

What is the expected behavior?

Since the request was already aborted, we shouldn't be hitting the handlers or anything.

Environment in which you encounter the issue:

  • mock-xmlhttprequest version: 7.0.4
  • Node.js version: N/A
  • OS version:

Did this work in a previous version of mock-xmlhttprequest (which)?

Do you intend to provide a pull request to address this issue?

Forked On 14 Jul 2022 at 04:56:58

Jamie5

Are you actually allowed to call send twice on the same request? I believe MockXhr throws in that case and https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send also claims that calling send twice results in an exception.

As for why not just newMockXhr directly, I guess the quick start seemed reasonable enough so went with that and didn't read on. And I guess it does say it's mostly noted for historical reasons.

Commented On 14 Jul 2022 at 04:56:58

Jamie5

build

Pushed On 14 Jul 2022 at 04:37:35

Jamie5

x

Pushed On 14 Jul 2022 at 12:17:39
Pull Request

Jamie5

Expose function to set viewport

Created On 13 Jul 2022 at 11:20:25