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

Fix memory leak and potential props clash #18

Merged
merged 1 commit into from
Jun 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/pink-kiwis-flash.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@threlte/test': patch
---

Fix memory leak and potential props clash
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"license": "MIT",
"version": "0.2.3",
"scripts": {
"all": "npm run check && npm run lint && npm run test && npm run build",
"dev": "vite dev",
"build": "vite build && npm run package",
"preview": "vite preview",
Expand Down
5 changes: 4 additions & 1 deletion src/lib/Container.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
/** @type {typeof import('svelte').SvelteComponent} */
export let component

/** @type {Record<string, unknown> | undefined} */
export let componentProps

/** @type {import('svelte').SvelteComponent | undefined} */
export let ref = undefined

Expand Down Expand Up @@ -65,5 +68,5 @@
</script>

<SceneGraphObject object={threlteContext.scene}>
<svelte:component this={component} bind:this={ref} {...$$restProps} />
<svelte:component this={component} bind:this={ref} {...componentProps} />
</SceneGraphObject>
29 changes: 19 additions & 10 deletions src/lib/pure.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,18 @@ import Container from './Container.svelte'
import { Core } from './core.svelte.js'

/**
* Check if a value is a plain object.
*
* @param {unknown} maybeObj
* @returns {maybeObj is object}
* @returns {maybeObj is Record<string, unknown>}
*/
const isObject = (maybeObj) => {
return typeof maybeObj === 'object' && maybeObj !== null
return typeof maybeObj === 'object' && maybeObj !== null && !Array.isArray(maybeObj)
}

/** @type {Set<HTMLElement>} */
const targetCache = new Set()

/** @type {Set<HTMLCanvasElement>} */
const canvasCache = new Set()

/** @type {Set<Svelte.SvelteComponent>} */
const componentCache = new Set()

Expand All @@ -28,9 +26,13 @@ const componentCache = new Set()
/**
*
* @param {Record<string, unknown>} options
* @returns {Record<string, unknown> & { target?: HTMLElement }}
* @returns {Record<string, unknown> & { target?: HTMLElement, props?: Record<string, unknown> }}
*/
const checkProps = (options) => {
if (!isObject(options)) {
throw new TypeError('Render options must be a plain object')
}

const keys = Object.keys(options)
const isProps = !keys.some((option) => {
return Core.componentOptions.includes(option)
Expand All @@ -43,7 +45,7 @@ const checkProps = (options) => {
})

if (unrecognizedOptions.length > 0) {
throw Error(`
throw new TypeError(`
Unknown options were found [${unrecognizedOptions}]. This might happen if you've mixed
passing in props with Svelte options into the render function. Valid Svelte options
are [${Core.componentOptions}]. You can either change the prop names, or pass in your
Expand All @@ -52,6 +54,10 @@ const checkProps = (options) => {
`)
}

if (options.props && !isObject(options.props)) {
throw new TypeError('`props` option must be a plain object')
}

return options
}

Expand Down Expand Up @@ -95,22 +101,24 @@ export const render = (Component, componentOptions = {}, renderOptions = {}) =>

/** @type {HTMLCanvasElement} */
const canvas = renderOptions.canvas ?? document.createElement('canvas')
canvasCache.add(canvas)

/** @type {any} */
const ComponentConstructor = 'default' in Component ? Component.default : Component

/** @type {any} */
const anyContainer = Container

/** @type {Record<string, unknown> | undefined} */
let componentProps = checkedOptions.props

const component = Core.renderComponent(
anyContainer,
{
...checkedOptions,
props: {
...(isObject(checkedOptions.props) ? checkedOptions.props : {}),
canvas,
component: ComponentConstructor,
componentProps,
userSize: renderOptions.userSize,
},
target,
Expand Down Expand Up @@ -175,7 +183,8 @@ export const render = (Component, componentOptions = {}, renderOptions = {}) =>
* @param {Record<string, unknown>} props
*/
rerender: async (props) => {
Core.updateProps(component, props)
componentProps = { ...componentProps, ...props }
Core.updateProps(component, { componentProps })
await Svelte.tick()
},

Expand Down