Skip to content

Conversation

@tanhauhau
Copy link
Member

Fixes #5270

this caused by the difference between setAttribute vs setting the property using propertyName.

not really sure is this the right way of handling it? if so, should we also handle all the other attribute that has a custom propertyName in

?

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases, features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests with npm test or yarn test)

@Conduitry
Copy link
Member

The CI failure here seems to be legit. I'm seeing spread-element-input-bind-group-with-value-attr fail locally.

@tanhauhau
Copy link
Member Author

i've fixed the failed test case.

i start to feel that special logic for DOM elements that has special case is dispersed at various places, eg: attribute, bindings, and spread, and we are slowly copying from each other as someone found more edge cases...

I'm thinking of consolidating them into a new class, such as TextInput, RadioInput, CheckboxInput that keeps logic for rendering attributes like value, checked, which can be composed within Attribute, Binding and Spread.

what do you think?

@Conduitry
Copy link
Member

I'd be worried about changing the type fields in the AST, but if we could do this with regular class inheritance (without changing the AST shape), that sounds reasonable to me.

@Conduitry Conduitry merged commit c752ed3 into sveltejs:master Aug 27, 2020
@tanhauhau tanhauhau deleted the tanhauhau/gh-5270 branch August 27, 2020 21:28
taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
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.

Problem when using $$restProps and dynamic components

2 participants