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

chore: remove attribute mutation restrictions #1362

Merged
merged 1 commit into from
Jun 19, 2019

Conversation

ekashida
Copy link
Member

@ekashida ekashida commented Jun 19, 2019

Details

It doesn't make sense to restrict attribute mutation methods because authors are responsible for reflecting content attributes when they override default descriptors for global html attributes.

This PR also removes some error logging done in setAttribute because those errors are meant for properties.

Fixes #1257

Does this PR introduce breaking changes?

  • No, it does not introduce breaking changes.

// Check that the attribute name of the global property is the same as the
// attribute name being set by setAttribute.
if (attrName === globalAttrName) {
const { error } = globalHTMLProperties[propName];
Copy link
Member Author

Choose a reason for hiding this comment

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

These errors are meant for when global HTML properties are accessed (e.g., this.dataset).

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 2467e0f | Target commit: 73e79a3

lwc-engine-benchmark

table-append-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table/append/1k duration 152.75 (±4.40 ms) 151.40 (±5.15 ms) -1.3ms (0.9%) 👌
table-clear-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table/clear/1k duration 11.70 (±0.90 ms) 10.85 (±0.90 ms) -0.8ms (7.3%) 👍
table-create-10k metric base(2467e0f) target(73e79a3) trend
benchmark-table/create/10k duration 921.45 (±8.10 ms) 917.45 (±7.20 ms) -4.0ms (0.4%) 👌
table-create-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table/create/1k duration 115.65 (±3.25 ms) 114.60 (±2.75 ms) -1.1ms (0.9%) 👌
table-update-10th-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table/update-10th/1k duration 79.80 (±4.90 ms) 81.45 (±5.30 ms) +1.6ms (2.1%) 👎
tablecmp-append-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table-component/append/1k duration 245.40 (±13.45 ms) 242.00 (±16.65 ms) -3.4ms (1.4%) 👌
tablecmp-clear-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table-component/clear/1k duration 7.90 (±1.55 ms) 7.40 (±1.25 ms) -0.5ms (6.3%) 👌
tablecmp-create-10k metric base(2467e0f) target(73e79a3) trend
benchmark-table-component/create/10k duration 1809.50 (±9.65 ms) 1797.90 (±16.05 ms) -11.6ms (0.6%) 👍
tablecmp-create-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table-component/create/1k duration 200.60 (±5.10 ms) 199.30 (±6.25 ms) -1.3ms (0.6%) 👌
tablecmp-update-10th-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table-component/update-10th/1k duration 71.40 (±7.00 ms) 72.80 (±5.05 ms) +1.4ms (2.0%) 👌
wc-append-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table-wc/append/1k duration 281.10 (±6.20 ms) 277.15 (±10.65 ms) -4.0ms (1.4%) 👌
wc-clear-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table-wc/clear/1k duration 13.60 (±1.60 ms) 13.10 (±2.15 ms) -0.5ms (3.7%) 👌
wc-create-10k metric base(2467e0f) target(73e79a3) trend
benchmark-table-wc/create/10k duration 2235.10 (±13.25 ms) 2231.00 (±13.90 ms) -4.1ms (0.2%) 👌
wc-create-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table-wc/create/1k duration 267.65 (±5.70 ms) 262.45 (±7.15 ms) -5.2ms (1.9%) 👍
wc-update-10th-1k metric base(2467e0f) target(73e79a3) trend
benchmark-table-wc/update-10th/1k duration 71.35 (±4.90 ms) 72.15 (±6.15 ms) +0.8ms (1.1%) 👌

@ekashida ekashida merged commit ab20a06 into master Jun 19, 2019
@ekashida ekashida deleted the ekashida/remove-restrictions branch June 19, 2019 21:02
ravijayaramappa added a commit that referenced this pull request Mar 4, 2020
ravijayaramappa added a commit that referenced this pull request Mar 6, 2020
ravijayaramappa added a commit that referenced this pull request Mar 11, 2020
* test: remove unit tests related to global attribute restriction
restriction was removed via #1362
* test: update error message in scoped-ids.spec.ts
* test: update test to match <slot> element spec
* test: remove unit test that was testing non-existent functions
* test: migrate tests to integration-karma
* test: migrate scoped-ids.spec to karma suite
* test: migrate html-element.spec.ts to karma
* test: migrate traverse.spec.ts to karma suite
ekashida pushed a commit that referenced this pull request Apr 17, 2020
* test: remove unit tests related to global attribute restriction
restriction was removed via #1362
* test: update error message in scoped-ids.spec.ts
* test: update test to match <slot> element spec
* test: remove unit test that was testing non-existent functions
* test: migrate tests to integration-karma
* test: migrate scoped-ids.spec to karma suite
* test: migrate html-element.spec.ts to karma
* test: migrate traverse.spec.ts to karma suite

(cherry picked from commit c1b56ec)
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.

Incorrect "invalid attribute access" error when running jest tests
2 participants