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(engine): live bindings for value and checked properties 3nd attempt #340

Conversation

caridy
Copy link
Contributor

@caridy caridy commented May 24, 2018

Details

Follow up on #312.

As today, manual mutations in regular HTML element are allowed, while manual mutations on custom elements are not allowed, arguing that the element is controlled by the template, and mutations can only be carry on by it.

With this PR we are relaxing this, and only giving them a warning with more details. This facilitate authors to force pushing new values down to other elements that they own.

Does this PR introduce a breaking change?

  • No

@caridy caridy requested a review from diervo May 24, 2018 20:01
assert.logError(`Invalid attempt to set property ${key} from ${vm} to ${newValue}. This property was decorated with @api, and can only be changed via the template.`);
}
vmBeingUpdated = null; // releasing the lock
// not need to wrap or check the value since that is happening somewhere else
Copy link

Choose a reason for hiding this comment

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

not need = no need

}
} else if (process.env.NODE_ENV !== 'production') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this condition was moved up to only run when vmBeingUpdated is not the current vm, which means it is not a controlled mutation. In which case we warn.

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 1e4c7f1 | Target commit: 440b8b4

lwc-engine-benchmark

table-append-1k metric base(1e4c7f1) target(440b8b4) trend
benchmark-table/append/1k duration 147.90 (± 4.80 ms) 147.60 (± 4.10 ms) 0.20% 👌
table-clear-1k metric base(1e4c7f1) target(440b8b4) trend
benchmark-table/clear/1k duration 12.00 (± 0.40 ms) 12.00 (± 0.60 ms) 0.00% 👌
table-create-10k metric base(1e4c7f1) target(440b8b4) trend
benchmark-table/create/10k duration 865.40 (± 6.00 ms) 848.30 (± 4.70 ms) 1.98% 👍
table-create-1k metric base(1e4c7f1) target(440b8b4) trend
benchmark-table/create/1k duration 101.40 (± 1.30 ms) 100.90 (± 1.90 ms) 0.49% 👌
table-update-10th-1k metric base(1e4c7f1) target(440b8b4) trend
benchmark-table/update-10th/1k duration 89.30 (± 4.20 ms) 87.00 (± 5.40 ms) 2.58% 👌
tablecmp-append-1k metric base(1e4c7f1) target(440b8b4) trend
benchmark-table-component/append/1k duration 250.00 (± 4.10 ms) 247.10 (± 4.30 ms) 1.16% 👍
tablecmp-clear-1k metric base(1e4c7f1) target(440b8b4) trend
benchmark-table/clear/1k duration 33.45 (± 1.45 ms) 32.50 (± 1.50 ms) 2.84% 👍
tablecmp-create-10k metric base(1e4c7f1) target(440b8b4) trend
benchmark-table-component/create/10k duration 1709.40 (± 7.80 ms) 1720.00 (± 8.50 ms) -0.62% 👎
tablecmp-create-1k metric base(1e4c7f1) target(440b8b4) trend
benchmark-table-component/create/1k duration 193.90 (± 5.10 ms) 193.90 (± 3.70 ms) 0.00% 👌
tablecmp-update-10th-1k metric base(1e4c7f1) target(440b8b4) trend
benchmark-table-component/update-10th/1k duration 74.00 (± 3.30 ms) 74.90 (± 2.60 ms) -1.22% 👌

Copy link
Contributor

@davidturissini davidturissini left a comment

Choose a reason for hiding this comment

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

This will address the issue in question, but I'm worried it'll open us up to more problems/footguns

@trevor-bliss trevor-bliss merged commit ef4acff into master May 24, 2018
@pmdartus pmdartus deleted the caridy/fix-live-bindings-value-and-checked-props-third-attempt branch June 18, 2018 22:59
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.

3 participants