fix(css-compiler): relaxation of attribute css rules #1248
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jodarove
commented
May 20, 2019
| word: attributeName, | ||
| }); | ||
| } | ||
| function validateAttribute(root: Root, result: null | Result) { |
There was a problem hiding this comment.
This has the important logic, is difficult to see here in the diff because i replaced the root.walk() + if (isAttribute(node)) with the walkAttributes helper function.
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
reviewed
May 20, 2019
| [ | ||
| { | ||
| "type": "warning", | ||
| "text": "Invalid usage of attribute selector \"custom-attribute\". Attribute \"custom-attribute\" is not a known attribute on <x-foo> element.", |
There was a problem hiding this comment.
this warning should probably change to something less strong... it is saying invalid at the moment.
caridy
suggested changes
May 20, 2019
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
reviewed
May 21, 2019
| @@ -1,7 +1,7 @@ | |||
| [ | |||
| { | |||
| "type": "warning", | |||
| "text": "Invalid usage of attribute selector \"custom-attribute\". Attribute \"custom-attribute\" is not a known attribute on <x-foo> element.", | |||
| "text": "Attribute \"custom-attribute\" is not a known attribute on <x-foo> element.", | |||
There was a problem hiding this comment.
is an unknown
maybe @ekashida or @jbleyleSF can help with the wording of the error
ekashida
reviewed
May 21, 2019
There was a problem hiding this comment.
I have an issue with the proposed changes because it feels like we're taking a step backwards in terms of user experience. We've already seen that warnings are difficult to manage and users learn to ignore warnings as soon as we introduce noise. I would rather see a solution where we either 1) abandon this type of validation, 2) keep the errors but introduce a way for the user to have the compiler ignore it via ignore comments, or 3) as @pmdartus has mentioned elsewhere, move this validation to a CSS linter.
It seems that neither (1) nor (2) are blockers for the ideal solution (3). Maybe we can just remove everything for now?
| 'lwc:dom', | ||
| 'if:true', | ||
| 'if:false', | ||
| 'for:each', |
There was a problem hiding this comment.
Missing:
for:itemfor:indexiterator:*
There was a problem hiding this comment.
I think have a wildcard helps... something like lwc:* and for: and iterator:*
|
|
||
| const DEPRECATED_SELECTORS = new Set(['/deep/', '::shadow', '>>>']); | ||
| const UNSUPPORTED_SELECTORS = new Set(['::slotted', ':root', ':host-context']); | ||
| const LWC_RESERVED_ATTRIBUTE_SELECTORS = new Set([ |
There was a problem hiding this comment.
I think we should be referring to these as "template directives" here and in the corresponding error messages since that they are not normal attributes.
Removes the validation rule: unknown attribute without a tag.
Reason:
Before: if you have a rule .foo[disabled]: {}, the css compiler would throw with message Invalid usage of attribute selector disabled. For validation purposes, attributes that are not global attributes must be associated with a tag name when used in a CSS selector.
This rule can be valid for simplicity purposes, the disabled attribute is valid in button, fieldset, input etc. thus having this one rule makes sense, with the restriction you would need 3 (or more) rules
removes css attribute restriction: unknownattribute on known element
jodarove
force-pushed
the
jodarove/css-remove-attribute-validation
branch
from
b42e100 to
4029500
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
pmdartus
approved these changes
May 24, 2019
ekashida
reviewed
May 24, 2019
|
|
||
| if (isTemplateDirective) { | ||
| const message = [ | ||
| `Invalid usage of attribute selector "${attributeName}". `, |
There was a problem hiding this comment.
Invalid attribute selector "${attributeName}".
There was a problem hiding this comment.
As i see it, is not that the attribute selector is invalid, is that we don't allow it to be used in LWC.
| if (isTemplateDirective) { | ||
| const message = [ | ||
| `Invalid usage of attribute selector "${attributeName}". `, | ||
| `"${attributeName}" is a template directive and therefore not supported in css rules.`, |
There was a problem hiding this comment.
Template directives cannot be used as selectors because they are not real attributes.
There was a problem hiding this comment.
i dont completely agree with not real attributes, but if this makes more sense to customers than the proposed, im fine changing the message. thoughts @jbleyleSF ?
caridy
approved these changes
May 24, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Addresses issue #1219
Without this PR, the css compilation currently fails with:
1- Unknown attributes in known selectors:
2- Unknown attributes without a tag:
3- Attributes in custom elements (is a consequence of (1))
With this PR, we:
[key='entity-id'] {}or[iterator:iteratorName] {}Does this PR introduce a breaking change?