Ensure unsupported arguments to <Input> and <LinkTo> issue warnings or errors appropriately
#77
Conversation
There was a problem hiding this comment.
@rwjblue I am afraid I need a bit of guidance here, see my comments below...
lib/helpers/check-attributes.js
Outdated
| const supportsAttribute = require('./supports-attribute'); | ||
|
|
||
| module.exports = function checkAttributes(node, supported) { | ||
| let sourceReference = calculateLocationDisplay(null, node.loc.start); |
There was a problem hiding this comment.
Passing null here as the moduleName, because I didn't find a way to access the template file name from within the AST transform. Any idea how this could work?
I am trying to give a useful hint to where the actual violation is located. Throwing an error will at least show the offending file downstream, but not for the warning obviously...
There was a problem hiding this comment.
@rwjblue any idea about this?
There was a problem hiding this comment.
Done now as suggested.
simonihmig
force-pushed
the
assert-builtin-components
branch
from
9f3c02b
to
3042d8a
Compare
| assert.dom('input').exists(); | ||
| }); | ||
|
|
||
| test('it ignores unknown attributes', async function(assert) { |
There was a problem hiding this comment.
This test doesn't seem quite correct, the assertion doesn't check to confirm that data-test-foo or aria-labelledby are missing on the input.
Also, once you update the tests to do those assertions you'll need to guard the test for <= 3.11 (because as of 3.11 this does work due to built-in support).
There was a problem hiding this comment.
Yeah, I thought about this. And agree the test is not that useful. Ideally it would assert that we see a warning, but as that happens at build-time, I don't see an easy way to test that.
I was mainly writing that test to ensure that passing unsupported attributes at least does not blow up, and to see the warning when running it locally on my machine.
Adding an assertion that the attribute is missing feels awkward to me tbh, as ideally we would support arbitrary attributes, just as native angle brackets do (if we had a good way to polyfill that). So seeing tests as a way to formally describe the features of our piece of software, in this case we would not describe and test for a feature, but a missing feature, right? Which seems odd. Or in other words, should we find a way to polyfill support for arbitrary attributes, that added feature should not break any tests!
Just my 2¢ though...
There was a problem hiding this comment.
I was mainly writing that test to ensure that passing unsupported attributes at least does not blow up, and to see the warning when running it locally on my machine.
Ya, this makes sense.
So seeing tests as a way to formally describe the features of our piece of software, in this case we would not describe and test for a feature, but a missing feature, right? Which seems odd.
Totally agree.
Fixes the remaining issue in ember-polyfills#66.
simonihmig
force-pushed
the
assert-builtin-components
branch
from
3042d8a
to
0c7801a
Compare
rwjblue
changed the title
rwjblue
changed the title
<Input> and <LinkTo> issue warnings or errors appropriately
rwjblue
added
bug
Fixes the remaining issue in #66.