-
Notifications
You must be signed in to change notification settings - Fork 191
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
Add a compile time error when passing arguments to regular HTML elements (e.g. <a @foo=
)
#1102
Add a compile time error when passing arguments to regular HTML elements (e.g. <a @foo=
)
#1102
Conversation
3abccbf
to
f27f3f7
Compare
Adds an error message in the compile phase to inform the developer that `@arguments` are not allowed on HTML nodes. This can often come from copy/pasting refactoring errors. For example, given the following template: ```handlebars <a href="https://emberjs.com" @OnClick={{action "foo"}}>Ember.js</a> ``` Before, the developer would get a vague error at runtime ([Example taken from ember.js#18951](emberjs/ember.js#18951)): ``` jquery.js:3827 Uncaught TypeError: func is not a function at StatementCompilers.compile (VM32 ember.debug.js:43121) at compileStatements (VM32 ember.debug.js:44323) at maybeCompile (VM32 ember.debug.js:44312) at CompilableTemplateImpl.compile (VM32 ember.debug.js:44292) at Object.evaluate (VM32 ember.debug.js:51011) at AppendOpcodes.evaluate (VM32 ember.debug.js:49382) at LowLevelVM.evaluateSyscall (VM32 ember.debug.js:52284) at LowLevelVM.evaluateInner (VM32 ember.debug.js:52240) at LowLevelVM.evaluateOuter (VM32 ember.debug.js:52232) at JitVM.next (VM32 ember.debug.js:53175) ``` Now, they get an error message that looks like: ``` SyntaxError: ${attribute.name} is not a valid attribute name. @arguments are only allowed on components, but the tag for this element (\`${elementNode.tag}\`) is a regular, non-component HTML element. (location line and column) ``` Given that there's no special behavior glimmer adds for `@arguments` on regular HTML nodes, and it seems invalid to put `@` in attribute names in regular HTML (validator.w3c.org gives ` Attribute @OnClick is not serializable as XML 1.0` error message for the example template below), it seems helpful for the glimmer compiler to fail at build time to prevent copy/paste errors and mistakes. Fixes emberjs/ember.js#18951
f27f3f7
to
f265f28
Compare
@@ -55,6 +55,19 @@ function test(desc: string, template: string, ...expectedStatements: BuilderStat | |||
const Append = Builder.Append; | |||
const Concat = Builder.Concat; | |||
|
|||
QUnit.test( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't match the test
style in the rest of the file, which is a wrapper around QUnit.test
and looks at wire format output. i thought it might be more clear to just use QUnit.test
directly rather than extend the test
function in this file to allow a callback/expect errors. Totally open to change this to whatever the maintainers prefer, let me know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good to me, thanks for working on it @fivetanley!
Cross linking some of the other convo on this subject: ember-template-lint/ember-template-lint#927. Also somewhat related to ember-template-lint/ember-template-lint#1334 (though not 100%).
<a @foo=
)
Adds an error message in the compile phase to inform the developer that
@arguments
are not allowed on HTML nodes. This can often come fromcopy/pasting refactoring errors.
For example, given the following template:
Before, the developer would get a vague error at runtime (Example taken from
ember.js#18951):
Now, they get an error message that looks like:
Given that there's no special behavior glimmer adds for
@arguments
on regularHTML nodes, and it seems invalid to put
@
in attribute names in regular HTML(validator.w3c.org gives
Attribute @onclick is not serializable as XML 1.0
error message for the example template above), it seems helpful for the glimmer
compiler to fail at build time to prevent copy/paste errors and mistakes.
Fixes emberjs/ember.js#18951