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

Improve typecheck for svelte:component bind:this #1214

Closed
wants to merge 2 commits into from
Closed

Improve typecheck for svelte:component bind:this #1214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
484c12d
Improve typecheck for bind:this, especially on svelte:component
mvolfik Oct 17, 2021
f46480e
Add explanation & fixup tests for improved svelte:component bind:this…
mvolfik Oct 23, 2021
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
23 changes: 22 additions & 1 deletion packages/svelte2tsx/src/htmlxtojsx/nodes/binding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,28 @@ export function handleBinding(
if (attr.name === 'this' && supportsBindThis.includes(el.type)) {
const thisType = getThisType(el);

if (thisType) {
if (el.type === 'InlineComponent') {
// bind:this is effectively only works bottom up - the variable is updated by the element, not
// the other way round. So we check if the instance is assignable to the variable. We get the
// instance from the class with instanceOf. The class for svelte:component can be anything
// specified in the this={x} expression - so we copy its contents.
// TODO: For svelte:self, getThisType returns effectively nothing, so it's not typechecked
// In other cases, it's just the Component class.
//
// If the component unmounts (it's inside an if block, or svelte:component this={null},
// the value also becomes null, so we add that to the clause as well.

str.overwrite(attr.start, attr.expression.start, '{...__sveltets_1_empty((');
str.appendLeft(attr.expression.end, ' = __sveltets_1_instanceOf(');
if (el.name === 'svelte:component') {
str.copy(el.expression.start, el.expression.end, attr.expression.end);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we need copy here, I don't see the need to have both mappings, the instanceOf(Component) thing does not need a reference to the original. Rename will work as it will add changes to the Svelte file, which does only have the this={Component} reference, which will trigger another Svelte->tsx transformation. Diagnostics will probably not span the whole element but that's acceptable to me (or could be handled specifically)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm sorry, i don't understand what you're saying. could you please explain it better/use other words?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I expressed myself poorly here 😄 Maybe we turn it around: Could you explain why you think we need copy here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my thought process:

  • bind:this assigns a reference to the instance to the specified variable
  • for normal components, the instance is just instanceof(ComponentClass)
  • however, for svelte:component this={expression}, the expression could be anything, so it's instanceOf whatever is in the this={} clause
  • I have no idea if there's some typechecking on the this={} - but there certainly could be something checking if it's [a component class or null], so I don't want to move it away, but instead copy

so if you were asking is why am I choosing to 'smart'-copy it with source mapping instead of just appending the string just like when it's a normal component, well, I have honestly no idea - it just looked like the right way to do it

} else {
str.appendLeft(attr.expression.end, thisType);
}
str.overwrite(attr.expression.end, attr.end, ') || null))}');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure on this change, it might break some people's code, and if it's not inside an if and not svelte:component with dynamic this, it's not true. Suppose you are in strict mode, then you could type the binding variable as let component: Component | undefined and this breaks for you (null !== undefined).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I'll remove it. people using this inside {#if} will hopefully be aware, and needless strictness over null|undefineds is annoying


return;
} else if (thisType) {
str.remove(attr.start, attr.expression.start);
str.appendLeft(attr.expression.start, `{...__sveltets_1_ensureType(${thisType}, `);
str.overwrite(attr.expression.end, attr.end, ')}');
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte2tsx/test/htmlx2jsx/samples/binding-this-component/expected.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<><Component type="radio" {...__sveltets_1_ensureType(Component, element)} value="Plain"/></>
<><Component type="radio" {...__sveltets_1_empty((element = __sveltets_1_instanceOf(Component) || null))} value="Plain"/></>
2 changes: 1 addition & 1 deletion packages/svelte2tsx/test/htmlx2jsx/samples/binding-this-svelte-component/expected.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<><sveltecomponent this={A} {...__sveltets_1_ensureType(__sveltets_1_componentType(), element)} /></>
<><sveltecomponent this={A} {...__sveltets_1_empty((element = __sveltets_1_instanceOf(A) || null))} /></>
2 changes: 1 addition & 1 deletion packages/svelte2tsx/test/htmlx2jsx/samples/binding-this-svelte-self/expected.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<>{(false) ? <>
<svelteself {...__sveltets_1_ensureType(__sveltets_1_componentType(), element)} />
<svelteself {...__sveltets_1_empty((element = __sveltets_1_instanceOf(__sveltets_1_componentType()) || null))} />
</> : <></>}</>
10 changes: 5 additions & 5 deletions packages/svelte2tsx/test/sourcemaps/samples/component-props/mappings.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@

<Component {/**
------------------------------------------------------------------------------------------------------------------------------------------------------ */}
{...__sveltets_1_ensureType(Component, bar)} {/**
╚{...__sveltets_1_ensureType(Component,•bar)}↲ [generated] line 18
╚ bar} ↲
╚ bar}↲
╚bind:this={bar}↲ [original] line 16
{...__sveltets_1_empty((bar = __sveltets_1_instanceOf(Component) || null))} {/**
╚{...__sveltets_1_empty((bar•=•__sveltets_1_instanceOf(Component)•||•null))}↲ [generated] line 18
╚b bar } ↲
╚b bar}↲
╚bind:this={bar}↲ [original] line 16
------------------------------------------------------------------------------------------------------------------------------------------------------ */}
/></> {/**
/></>↲ [generated] line 19
Expand Down
10 changes: 5 additions & 5 deletions packages/svelte2tsx/test/sourcemaps/samples/large-sample-1/mappings.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,11 +400,11 @@ s
<div class="tutorial-repl">
<Repl {/**
------------------------------------------------------------------------------------------------------------------------------------------------------ */}
{...__sveltets_1_ensureType(Repl, repl)} {/**
╚╚╚╚{...__sveltets_1_ensureType(Repl,•repl)}↲ [generated] line 163
╚╚╚╚ repl} ↲
╚╚╚╚ repl}↲
╚╚╚╚bind:this={repl}↲ [original] line 303
{...__sveltets_1_empty((repl = __sveltets_1_instanceOf(Repl) || null))} {/**
╚╚╚╚{...__sveltets_1_empty((repl•=•__sveltets_1_instanceOf(Repl)•||•null))}↲ [generated] line 163
╚╚╚╚b repl } ↲
╚╚╚╚b repl}↲
╚╚╚╚bind:this={repl}↲ [original] line 303
------------------------------------------------------------------------------------------------------------------------------------------------------ */}
workersUrl="workers" {/**
------------------------------------------------------------------------------------------------------------------------------------------------------ */}
Expand Down