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

feat: add support for resize observer bindings #8022

Merged
merged 17 commits into from
Apr 11, 2023

Conversation

TheCymaera
Copy link
Contributor

@TheCymaera TheCymaera commented Nov 10, 2022

Implements ResizeObserver bindings: #5524 (comment)
Continuation of: #5963
Related to #7583

<div bind:contentRect|contentBoxSize|borderBoxSize|devicePixelContentBoxSize>

I'm not familiar with the codebase so I'm not sure if this is the intended way to implement special bindings.

@TheCymaera
Copy link
Contributor Author

TheCymaera commented Nov 10, 2022

I made a different pull request earlier using resize observers for the old dimension bindings.

I removed them as I was told that it would cause inconsistent behaviour between browsers.

With your permission, I'd like to re-implement it behind a compiler option, as the old bindings are easier to use.

resizeDetection: "iframe"|"resizeObserver"

@TheCymaera
Copy link
Contributor Author

TheCymaera commented Nov 24, 2022

Hi all. I need some advice on how to fix this error. It is happening during node ./generate-type-definitions.js and is being caused by references to the ResizeObserver types. I was able to suppress it using the following but I need a way to fix it properly.

type ResizeObserverEntry = any;
type ResizeObserverOptions = any;
type ResizeObserver = any;
declare const ResizeObserver: any;

I tried changing the TSConfig version to ES2020 with no luck. I'm also not getting any type errors in VSCode.

Should I just copy the ResizeObserver type definitions from lib.dom.d.ts? That way we don't need to change lib versions.

@dummdidumm
Copy link
Member

What's the error exactly? I would be ok to slap a // @ts-expect-error <explanation why> comment above it if that fixes it

@TheCymaera
Copy link
Contributor Author

What's the error exactly? I would be ok to slap a // @ts-expect-error <explanation why> comment above it if that fixes it

Adding ts-expect-error prints Unused 'ts-expect-error' directive.
Adding ts-ignore or ts-nocheck doesn't fix it either.
This isn't a normal type error.

My above solution works and is probably fine since the observer is only used internally.

Error
> [email protected] tsd
> node ./generate-type-definitions.js

node:child_process:891
    throw err;
    ^

Error: Command failed: tsc -p src/compiler --emitDeclarationOnly && tsc -p src/runtime --emitDeclarationOnly
    at checkExecSyncError (node:child_process:817:11)
    at execSync (node:child_process:888:15)
    at Object.<anonymous> (/Users/morgan/Desktop/svelte/generate-type-definitions.js:6:1)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Module._load (node:internal/modules/cjs/loader:827:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47 {
  status: 1,
  signal: null,
  output: [
    null,
    Buffer(105) [Uint8Array] [
      115, 114,  99,  47, 114, 117, 110, 116, 105, 109, 101,  47,
      105, 110, 116, 101, 114, 110,  97, 108,  47,  82, 101, 115,
      105, 122, 101,  79,  98, 115, 101, 114, 118, 101, 114,  83,
      105, 110, 103, 108, 101, 116, 111, 110,  46, 116, 115,  40,
       50,  51,  44,  50,  50,  41,  58,  32, 101, 114, 114, 111,
      114,  32,  84,  83,  50,  51,  48,  52,  58,  32,  67,  97,
      110, 110, 111, 116,  32, 102, 105, 110, 100,  32, 110,  97,
      109, 101,  32,  39,  82, 101, 115, 105, 122, 101,  79,  98,
      115, 101, 114, 118,
      ... 5 more items
    ],
    Buffer(0) [Uint8Array] []
  ],
  pid: 70590,
  stdout: Buffer(105) [Uint8Array] [
    115, 114,  99,  47, 114, 117, 110, 116, 105, 109, 101,  47,
    105, 110, 116, 101, 114, 110,  97, 108,  47,  82, 101, 115,
    105, 122, 101,  79,  98, 115, 101, 114, 118, 101, 114,  83,
    105, 110, 103, 108, 101, 116, 111, 110,  46, 116, 115,  40,
     50,  51,  44,  50,  50,  41,  58,  32, 101, 114, 114, 111,
    114,  32,  84,  83,  50,  51,  48,  52,  58,  32,  67,  97,
    110, 110, 111, 116,  32, 102, 105, 110, 100,  32, 110,  97,
    109, 101,  32,  39,  82, 101, 115, 105, 122, 101,  79,  98,
    115, 101, 114, 118,
    ... 5 more items
  ],
  stderr: Buffer(0) [Uint8Array] []
}

Node.js v18.0.0
node:child_process:891
    throw err;

@benmccann benmccann changed the title [feat] ✨ Added support for resize observer bindings feat: add support for resize observer bindings Jan 12, 2023
@vercel
Copy link

vercel bot commented Mar 16, 2023

@dummdidumm is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@dummdidumm dummdidumm modified the milestones: 4.x, 3.x Apr 11, 2023
@dummdidumm dummdidumm merged commit 0adc09d into sveltejs:master Apr 11, 2023
@benmccann benmccann mentioned this pull request Apr 11, 2023
5 tasks
dummdidumm added a commit to dummdidumm/svelte that referenced this pull request Apr 11, 2023
@dummdidumm dummdidumm mentioned this pull request Apr 11, 2023
5 tasks
dummdidumm added a commit that referenced this pull request Apr 11, 2023
@Conduitry
Copy link
Member

This has been released in 3.59.0. Thank you!

@philholden
Copy link

Good to have this feature as it should be more performant but feels less ergonomic than clientWidth for getting hold of width:

Old:

<script>
  let clientWidth
</script>
<div bind:clientWidth>Width: {clientWidth}</div>

New (pretty hard to remember):

<script>
  let borderBoxSize;
  let width;
  $: width = borderBoxSize && borderBoxSize[borderBoxSize.length-1]?.inlineSize;
</script>
<div bind:borderBoxSize>Width {width}</div>

Any chance of some shorthand that removes this boilerplate e.g.:

<script>
  let borderBoxInlineSize;
</script>
<div bind:borderBoxInlineSize>Width: {borderBoxInlineSize}</div>

@aradalvand
Copy link

aradalvand commented Jul 26, 2023

Is this documented? It looks like it isn't.
I wouldn't have known this feature existed had I not stumbled into this PR, it should be mentioned in the docs.

@hackape
Copy link
Contributor

hackape commented Jul 26, 2023

@aradalvand Good catch! Would you like to submit a PR to supplement the docs?

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.

6 participants