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.

Sign up for GitHub

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

Jump to bottom

🐛 Assert that values are never returned from vsync callbacks #20836

Merged
merged 5 commits into from
Feb 14, 2019
Merged

🐛 Assert that values are never returned from vsync callbacks #20836

merged 5 commits into from
Feb 14, 2019

Conversation

jridgewell
Copy link
Contributor

Vsync does not propogate those values in any way. This is specifically targetting code like:

vsync.measure(() => {
  return el.getBoundingClientRect();
});

// or
vsync.mutatePromise(() => {
  return someOtherPromise;
});

@jridgewell
Assert that values are never returned from vsync callbacks
9fd6cc6 
Vsync does not propogate those values in any way. This is specifically targetting code like:

```js
vsync.measure(() => {
  return el.getBoundingClientRect();
});

// or
vsync.mutatePromise(() => {
  return someOtherPromise;
});
```
@dvoytenko
Copy link
Contributor

@jridgewell The change itself makes sense. I'm wondering if we should allow return values at some point however...

@jridgewell
Copy link
Contributor Author

I'm wondering if we should allow return values at some point however...

Only measurePromise and mutatePromise make sense for these cases, and measurePromise already works!

mutatePromise doesn't because we share the same Promise between multiple calls in the same phase. We could change that.

@dvoytenko
Copy link
Contributor

I agree. It makes a lot less sense for mutate to return value comparing to measure.

@jridgewell jridgewell merged commit 35d6030 into ampproject:master Feb 14, 2019
@jridgewell jridgewell deleted the vsync-no-returns branch February 14, 2019 21:02
estherkim added a commit to estherkim/amphtml that referenced this pull request Feb 15, 2019
@estherkim
Squashed commit of the following:
e199707 
commit 08466820bf2c8c13e21abc5c26159d228edb345c
Author: Esther Kim <[email protected]>
Date:   Fri Feb 15 10:47:56 2019 -0500

    Whitespace

commit cdfd719
Author: Esther Kim <[email protected]>
Date:   Fri Feb 15 10:34:13 2019 -0500

    Stop timer logging

commit 4b63cd7
Author: Esther Kim <[email protected]>
Date:   Fri Feb 15 10:08:59 2019 -0500

    Typos

commit 7db4b28
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 18:12:37 2019 -0500

    Review comments

commit ced9c5d
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 18:01:12 2019 -0500

    Review comments

commit 8fa0e88
Merge: e65dd15 893ff3f
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 17:29:46 2019 -0500

    Merge branch 'master' into travis-build-matrix

commit e65dd15
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 17:28:35 2019 -0500

    Review comments

commit d69c38a
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 17:08:20 2019 -0500

    Update logging

commit 7e4f701
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 17:00:38 2019 -0500

    Review changes

commit 893ff3f
Author: Aaron Turner <[email protected]>
Date:   Thu Feb 14 13:49:16 2019 -0800

    Return 400 when recaptcha example fails (ampproject#20668)

commit 3f0d6c6
Author: Enrique Marroquin <[email protected]>
Date:   Thu Feb 14 16:37:15 2019 -0500

    [amp-story] Allow more parameters for pan/zoom animations ✨ (ampproject#20778)

    * allow more parameters in for pan/zoom animations.

    * change attribute names

    * types

    * fix test

    * assert values for params, change attribute naming.

    * change attribute names do scale-start, scale-end.

    * hoist defaults to constants

commit 35d6030
Author: Justin Ridgewell <[email protected]>
Date:   Thu Feb 14 16:02:00 2019 -0500

    🐛 Assert that values are never returned from vsync callbacks (ampproject#20836)

    * Assert that values are never returned from vsync callbacks

    Vsync does not propogate those values in any way. This is specifically targetting code like:

    ```js
    vsync.measure(() => {
      return el.getBoundingClientRect();
    });

    // or
    vsync.mutatePromise(() => {
      return someOtherPromise;
    });
    ```

    * Fix dev call

    * Lint

    * Don't use devAssert so this stays in production

    * Type checks

commit b9ba432
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 15:52:54 2019 -0500

    Fix nit, hopefully fix chrome start

commit 3cb87fa
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 15:08:42 2019 -0500

    bash

commit 5d9844e
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 14:54:40 2019 -0500

    rm

commit b027eb0
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 14:39:52 2019 -0500

    Review comments

commit fe27d76
Author: sohanirao <[email protected]>
Date:   Thu Feb 14 11:38:43 2019 -0800

    SwG Release 0.1.22.44 (ampproject#20834)

commit 736d506
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 14:25:37 2019 -0500

    Install chrome for dist test job, add EXTENSIONS_CSS_MAP to build artifact zip

commit a36ee5d
Author: delima02 <[email protected]>
Date:   Thu Feb 14 14:17:42 2019 -0500

    🐛 Guard against getBoundingClientRect to prevent unspecified errors in IE. (ampproject#20731)

    * Guard against `Element.getBoundingClientRect`.

    * Refactor solution.

    * Check if element exists in the DOM.

    * Fix solution using `Node.isConnected`.

    * Correct the default value of the desired DOMRect

    * Patch getBoundingClientRect for unsupported browsers. Alternative solution

    * Refactor patch.

    * Fix - use assigned variable rect, previously unused.

    * Fix imports.

    * Minor fixes.

    * Make the return type of getBoundingClientRect be consistent with usages

    * File overview comment and a minor fix.

commit e9acbd2
Author: Cathy Zhu <[email protected]>
Date:   Fri Feb 15 03:17:37 2019 +0800

    Undo space reservation after successful height change (ampproject#20856)

commit ccea3e2
Author: Nikita Beloglazov <[email protected]>
Date:   Thu Feb 14 11:12:16 2019 -0800

    Export few data types from content-recommendation.js (ampproject#20854)

    It allows users of that file to use the types. Given that we return objects of these types from exported functions - exporting types themselves makes sense.

    Tested:
      presubmit

commit 80f9fa9
Author: Sepand Parhami <[email protected]>
Date:   Thu Feb 14 11:10:11 2019 -0800

    🐛Do not perform static layout for cloned nodes with static layout. (ampproject#20724)

commit a751cc7
Author: keithwrightbos <[email protected]>
Date:   Thu Feb 14 13:47:48 2019 -0500

    AdSense Fast Fetch delay 3 viewport experiment - respect loading strategy (ampproject#20866)

    * initial commit

    * modify dep-check

    * truly fix dep issue

commit 806db59
Author: Carlos Vializ <[email protected]>
Date:   Thu Feb 14 13:21:26 2019 -0500

    ✨Implement autoexpand for amp-form textarea element (ampproject#20772)

    * Add expanding textarea feature to amp-form

    * Add autoshrink behavior in addition to autoexpand

    * reset

    * reset2 it works

    * Does not stay at top

    * Prevent scrollbar from flashing before max-height

    * default autoshrink. Prevent shrinking if the textarea hasn't yet expanded

    * Use class style

    * Add manual test

    * Cleanup

    * Review feedback. Remove autoshrink attr. Lazy-load feature.

    * Encapsulate all textarea logic, including installation.

    * Mark expensive property accesses as OK

    * Warn and forbid textarea with initial overflow

    * Add tests for confidence. Fix presubmit errors.

commit cb95582
Author: Esther Kim <[email protected]>
Date:   Thu Feb 14 11:51:37 2019 -0500

    Separate PR jobs

commit 4679a4c
Author: Gabriel Majoulet <[email protected]>
Date:   Thu Feb 14 10:52:06 2019 -0500

    Displaying more specific messages when users have to resize their window, + refactoring. (ampproject#20835)

commit b9814f5
Author: keithwrightbos <[email protected]>
Date:   Thu Feb 14 10:43:42 2019 -0500

    initial commit (ampproject#20865)

commit e570d46
Author: Hongfei Ding <[email protected]>
Date:   Wed Feb 13 22:15:49 2019 -0800

    amp-ad-exit: use HostService if exists. (ampproject#20357)

    * amp-ad-exit: use HostService if exists.

    * naming & docs

    * rename host-api to host-service

    * lint

commit ecc3daa
Author: Cathy Zhu <[email protected]>
Date:   Thu Feb 14 13:34:08 2019 +0800

    Make is-my-pr-in-production-yet more prominent in our docs (ampproject#20857)

commit 12d441c
Author: Alan Orozco <[email protected]>
Date:   Wed Feb 13 13:44:50 2019 -0800

    ✨auto-lightbox: Discard tap actions with invalid references (ampproject#20789)

    Exposes `ActionService.hasResolvableAction` to:

      - Discard tap action in auto-lightbox criteria when it does not resolve to a valid target.
      - Fix `amp-lightbox-gallery` so it will properly traverse up the tree to discard targets, and not to discard those dynamically set.

commit dd4037d
Author: Nikita Beloglazov <[email protected]>
Date:   Wed Feb 13 13:25:13 2019 -0800

    Improve Google matched content logic to reflect adsbygoogle.js. (ampproject#20624)

    The commit introduces core-shared.js file which supposed to be shared between AMP and internal Google adsbygoogle.js codebase. The file has no dependencies on environment to make importing these files in other projects easier.

    In particular this commit adds support for publisher settings as described in https://support.google.com/adsense/answer/7533385

    Tested:
      unit tests
      manually on fast festh and delayed fetch

commit 76a7c0f
Author: keithwrightbos <[email protected]>
Date:   Wed Feb 13 16:10:58 2019 -0500

    initial commit (ampproject#20837)

commit f1fb33b
Author: keithwrightbos <[email protected]>
Date:   Wed Feb 13 15:37:39 2019 -0500

    initial commit (ampproject#20833)

commit 6831b63
Author: Nikita Beloglazov <[email protected]>
Date:   Wed Feb 13 12:37:10 2019 -0800

    Cleanup adsense amp-auto-ads holdout experiment. Close ampproject#9247. (ampproject#20774)

    It has been running for a while and we no longer need it. If someone decides to look at results in future - they can rollback this commit and restart experiment.

    Tested:
      presubmit

commit 4f3cd0d
Author: William Chou <[email protected]>
Date:   Wed Feb 13 15:28:26 2019 -0500

    Don't send amp-bind state to untrusted viewers (ampproject#20822)

    * Only send state for history updates to trusted viewers.

    * Add and fix tests.

    * Fix type check.

    * Fix presubmit.

commit ab467bc
Author: Carlos Vializ <[email protected]>
Date:   Wed Feb 13 14:26:35 2019 -0500

    ✅ Resolve some flakiness in e2e tests (ampproject#20794)

    * Resolve some flakiness in e2e tests

    * Unskip passing test

commit e13a531
Author: Aaron Labiaga <[email protected]>
Date:   Wed Feb 13 13:35:59 2019 -0500

    Make test-resource.js not flaky (ampproject#20514)

    Remove call count expectations on getRect method call which is the culprit of the flakiness of the tests mentioned. This should be fine as it doesn't take away from what's being primarily tested as it occurs in the afterEach call. Note that this is not a permanent fix but a temporary one to alleviate the flakiness. I will keep ampproject#16124 open.

    Tested locally and ran it several times in saucelabs.

commit 4a7282f
Author: Carlos Vializ <[email protected]>
Date:   Wed Feb 13 12:01:23 2019 -0500

    Skip date picker visual diff tests for flakes (ampproject#20809)

commit 703ab02
Author: William Chou <[email protected]>
Date:   Wed Feb 13 11:53:05 2019 -0500

    Clean up 'user-error-reporting' (ampproject#20779)

    * Clean up user-error-reporting.

    * Fix lint.

commit 229d909
Author: Alan Orozco <[email protected]>
Date:   Wed Feb 13 02:44:33 2019 -0800

    🛑 <amp-video-iframe> integration safeguards (ampproject#20826)

    - Prefix all frame-level errors with `<amp-video-iframe>` to let authors know source of integration mistakes.
    - Fail to `adopt` window twice, notify that script tag should only be included once.
    - Display valid listener types when trying to `listenTo('invalidFramework')`.
    - Don't let author `listenTo` twice.

commit 84f701f
Author: Ali Ghassemi <[email protected]>
Date:   Tue Feb 12 23:21:22 2019 -0800

    ✅ amp-youtube: add amp=1 to the url (ampproject#20821)

commit b58b6fa
Author: Alan Orozco <[email protected]>
Date:   Tue Feb 12 23:03:58 2019 -0800

    ✨ Set #amp=1 fragment in amp-video-iframe src (ampproject#20823)

    Docs say we do it, but we don't, so let's do it!

    Also cleaned up some tests to be clearer and to use `async/await`.

commit f92f627
Author: Justin Ridgewell <[email protected]>
Date:   Tue Feb 12 20:24:26 2019 -0500

    Optimize scopeSelector (ampproject#20819)

    https://jsbench.github.io/#e00c46505b5b875d2dcd5894d6e46adc

commit 0298883
Author: Alan Orozco <[email protected]>
Date:   Tue Feb 12 16:57:41 2019 -0800

    🐛Reset min dimensions on undock (ampproject#20817)

    Otherwise the video can be oversized and clipped when docked and resizing the window.

commit bcce892
Author: Caleb Cordry <[email protected]>
Date:   Tue Feb 12 16:33:05 2019 -0800

    📖amp story ads RTC & Targeting docs (ampproject#20818)
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
@jridgewell
🐛 Assert that values are never returned from vsync callbacks (ampproj…
f35cdbf 
…ect#20836)

* Assert that values are never returned from vsync callbacks

Vsync does not propogate those values in any way. This is specifically targetting code like:

```js
vsync.measure(() => {
  return el.getBoundingClientRect();
});

// or
vsync.mutatePromise(() => {
  return someOtherPromise;
});
```

* Fix dev call

* Lint

* Don't use devAssert so this stays in production

* Type checks
bramanudom pushed a commit to bramanudom/amphtml that referenced this pull request Mar 22, 2019
@jridgewell @bramanudom
🐛 Assert that values are never returned from vsync callbacks (ampproj…
ac38fc4 
…ect#20836)

* Assert that values are never returned from vsync callbacks

Vsync does not propogate those values in any way. This is specifically targetting code like:

```js
vsync.measure(() => {
  return el.getBoundingClientRect();
});

// or
vsync.mutatePromise(() => {
  return someOtherPromise;
});
```

* Fix dev call

* Lint

* Don't use devAssert so this stays in production

* Type checks
@ampprojectbot ampprojectbot mentioned this pull request Apr 9, 2020
Closed
This was referenced Apr 22, 2020
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced May 5, 2020
Closed
Closed
This was referenced May 7, 2020
Closed
Closed
Closed
@ampprojectbot ampprojectbot mentioned this pull request May 26, 2020
Closed
@ampprojectbot ampprojectbot mentioned this pull request Jun 15, 2020
Closed
@ampprojectbot ampprojectbot mentioned this pull request Jul 29, 2020
Closed
@ampprojectbot ampprojectbot mentioned this pull request Dec 6, 2020
Closed
This was referenced Jan 12, 2021
Closed
Closed
Closed
@ampprojectbot ampprojectbot mentioned this pull request Feb 1, 2021
Closed
This was referenced Mar 4, 2021
Open
Closed
This was referenced May 24, 2021
Open
Closed
Closed
This was referenced Aug 4, 2021
Open
Open
@ampprojectbot ampprojectbot mentioned this pull request Sep 14, 2021
Closed
@ampprojectbot ampprojectbot mentioned this pull request Mar 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvoytenko dvoytenko dvoytenko approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jridgewell @dvoytenko @googlebot