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

feat: Restrict usage of attributes in stylesheets #316

Merged
merged 2 commits into from
May 21, 2018

Conversation

pmdartus
Copy link
Member

@pmdartus pmdartus commented May 18, 2018

Details

This PR restricts the usage of attributes in the stylesheet:

  • global HTML attributes are permitted
  • known attributes selectors are permitted on known elements selectors
  • attribute selectors are banned for custom elements

Fix #261

Does this PR introduce a breaking change?

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:
Components won't compile if they already use attribute selectors in their stylesheet.

@pmdartus pmdartus requested a review from caridy May 18, 2018 03:02
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 6ad80ca | Target commit: 3b86567

lwc-engine-benchmark

table-append-1k metric base(6ad80ca) target(3b86567) trend
benchmark-table/append/1k duration 151.80 (± 6.00 ms) 144.00 (± 3.60 ms) 5.14% 👍
table-clear-1k metric base(6ad80ca) target(3b86567) trend
benchmark-table/clear/1k duration 11.90 (± 0.50 ms) 11.80 (± 0.40 ms) 0.84% 👌
table-create-10k metric base(6ad80ca) target(3b86567) trend
benchmark-table/create/10k duration 843.60 (± 6.50 ms) 845.50 (± 5.70 ms) -0.23% 👌
table-create-1k metric base(6ad80ca) target(3b86567) trend
benchmark-table/create/1k duration 102.60 (± 1.50 ms) 100.50 (± 1.80 ms) 2.05% 👍
table-update-10th-1k metric base(6ad80ca) target(3b86567) trend
benchmark-table/update-10th/1k duration 90.35 (± 5.55 ms) 88.90 (± 4.90 ms) 1.60% 👌
tablecmp-append-1k metric base(6ad80ca) target(3b86567) trend
benchmark-table-component/append/1k duration 213.10 (± 14.50 ms) 243.20 (± 4.90 ms) -14.12% 👎
tablecmp-clear-1k metric base(6ad80ca) target(3b86567) trend
benchmark-table/clear/1k duration 31.90 (± 1.40 ms) 31.90 (± 1.20 ms) 0.00% 👌
tablecmp-create-10k metric base(6ad80ca) target(3b86567) trend
benchmark-table-component/create/10k duration 1701.20 (± 11.20 ms) 1695.80 (± 8.80 ms) 0.32% 👍
tablecmp-create-1k metric base(6ad80ca) target(3b86567) trend
benchmark-table-component/create/1k duration 192.90 (± 4.10 ms) 191.50 (± 2.90 ms) 0.73% 👌
tablecmp-update-10th-1k metric base(6ad80ca) target(3b86567) trend
benchmark-table-component/update-10th/1k duration 77.50 (± 4.70 ms) 74.10 (± 3.80 ms) 4.39% 👍


export const GLOBAL_ATTRIBUTE_SET: Set<string> = new Set([
'role',
'accesskey',
Copy link
Contributor

Choose a reason for hiding this comment

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

creating css rules based on accesskey seems wrong. /cc @stefsullrew

'lang',
'slot',
'spellcheck',
'style',
Copy link
Contributor

Choose a reason for hiding this comment

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

creating css rules based on style attribute seems wrong. /cc @stefsullrew

'spellcheck',
'style',
'tabindex',
'title',
Copy link
Contributor

Choose a reason for hiding this comment

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

creating css rules based on title attribute seems wrong because this is supposed to be internationalized while the css doesn't. /cc @stefsullrew

]);

export const HTML_ATTRIBUTES_REVERSE_LOOKUP: { [attr: string]: string[] } = {
'xlink:href': [
Copy link
Contributor

Choose a reason for hiding this comment

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

@stefsullrew can you also help us to review this list to ban anything that we consider a bad practice when writing component's css.

Copy link
Contributor

@caridy caridy left a comment

Choose a reason for hiding this comment

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

This is good stuff. Remember, these rules are not arbitrary rules, they have a purpose, the purpose is to prevent people from doing the wrong thing or trip over by silly mistakes. So, I think we need to be more vigilant on what we will allow and what we will not allow rather than stick to the tee on the spec.

@pmdartus
Copy link
Member Author

I agree with you Caridy, but in this case, it's not a compiler concern. The compiler takes care of making sure the code runs as expected, the linter, on the other hand, make sure the code is compliant with the best practices.

We have been striving so far to find the right balance, and when we doubted we went down the restrict path. Here we want to prevent the code to compile even if the runtime execution will be correct.

Copy link
Contributor

@caridy caridy left a comment

Choose a reason for hiding this comment

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

Ok @pmdartus, I'm fine with that. Let's make sure that we have an issue open to track that linter work, and we can have the discussion about what to restrict there.

@pmdartus pmdartus merged commit 948fbb9 into master May 21, 2018
@pmdartus pmdartus deleted the pmdartus/restrict-css-selectors branch May 21, 2018 14:08
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