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

Add polyfill for <Input> and <Textarea> #75

Merged
merged 1 commit into from
Jul 22, 2019

Conversation

simonihmig
Copy link
Contributor

Based on #72, will rebase once that is merged.

@rwjblue
Copy link
Member

rwjblue commented Jul 14, 2019

Needs a rebase I think

@simonihmig
Copy link
Contributor Author

Rebased, but the Ember 2.12 scenario is failing. Will have to investigate...

@simonihmig simonihmig force-pushed the builtin-input branch 2 times, most recently from 76e12c7 to f8a5c5e Compare July 14, 2019 19:02
@simonihmig
Copy link
Contributor Author

Ok, that was a stupid mistake. Green now!

@rwjblue
Copy link
Member

rwjblue commented Jul 14, 2019

Thank you! Glad we had coverage 😄

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Reading through this made me think of another issue: passing modifiers, comments, or attributes other than the specific ones we handle should issue a helpful error. I think this is also going to be an issue over in the link to one so probably could share a util or something?

lib/ast-input-transform.js Outdated Show resolved Hide resolved
function transformAttributeValue(attributeValue) {
switch (attributeValue.type) {
case 'TextNode':
return b.string(attributeValue.chars);
Copy link
Member

Choose a reason for hiding this comment

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

Let’s propagate the original location info in these branches (so downstream error messages can use it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems I cannot do that with b.string(), can I?

https://github.com/glimmerjs/glimmer-vm/blob/master/packages/%40glimmer/syntax/lib/builders.ts#L564

Btw, as AST docs are quite sparse/non-existent, what does this actually do and how is this used? For error messages to print concrete line/column numbers of hbs files that break something?

Copy link
Member

Choose a reason for hiding this comment

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

It seems I cannot do that with b.string(), can I?

Ugg, indeed you are correct. Though I think you can create it and then add the loc:

let newString = b.string(attributeValue.chars);
newString.loc = attributeValue.loc;
return newString;

https://github.com/glimmerjs/glimmer-vm/blob/83a0c2fdfa8f22fd353dcab32ea41c4bcacaa379/packages/%40glimmer/syntax/lib/builders.ts#L577-L581 needs to be updated to take loc as the last arg in the returned function.

what does this actually do and how is this used? For error messages to print concrete line/column numbers of hbs files that break something?

Right, when creating new AST nodes we should always attempt to propagate the original location information through to the new nodes. This helps to ensure that any error messages include the correct module name, line, and column number.

@simonihmig
Copy link
Contributor Author

Reading through this made me think of another issue: passing modifiers, comments, or attributes other than the specific ones we handle should issue a helpful error.

What do you mean by "comments"?

So to make this clear, as we transform to curly syntax that does not support arbitrary HTML attributes and no modifiers (which RFC435 does allow for angle bracket invocation), we should error on

  • passing an attribute that is not supported through attributeBindings
  • passing any kind of modifier

However passing any kind of unsupported property should not error IMHO, as that wouldn't be the case also with "native" angle bracket support (no polyfill required).

Correct?

Do you want me to tackle this in this PR? I would suggest to do that in another one, that also includes the link-to transform. So we can hopefully merge this before?

@rwjblue
Copy link
Member

rwjblue commented Jul 22, 2019

Do you want me to tackle this in this PR? I would suggest to do that in another one, that also includes the link-to transform. So we can hopefully merge this before?

Sorry, yes, I totally agree that we don't want that here in this PR. I just thought of it while reviewing this one.

@rwjblue
Copy link
Member

rwjblue commented Jul 22, 2019

What do you mean by "comments"?

<LinkTo @route="foo" @model={{whatever}} {{! template-lint-disable-tree no-bare-strings }}>Hello Untranslated String!</LinkTo>

@rwjblue
Copy link
Member

rwjblue commented Jul 22, 2019

So to make this clear, as we transform to curly syntax that does not support arbitrary HTML attributes and no modifiers (which RFC435 does allow for angle bracket invocation), we should error on

  • passing an attribute that is not supported through attributeBindings
  • passing any kind of modifier

Yes, exactly what I meant.

However passing any kind of unsupported property should not error IMHO, as that wouldn't be the case also with "native" angle bracket support (no polyfill required).

I don't quite understand what you mean here. Can you give an example of what you refer to as an "unsupported property"? Do you mean an attribute that isn't in the attributeBindings? A named argument that we don't "know" about? Something else?

@rwjblue rwjblue merged commit 75b5840 into ember-polyfills:master Jul 22, 2019
@rwjblue rwjblue added the enhancement New feature or request label Jul 22, 2019
@simonihmig
Copy link
Contributor Author

A named argument that we don't "know" about?

Yes, that one. I probably was confused about your "comment" remark, because I didn't realize that was even possible until now, so thought you might mean some else / have a typo. I guess we agree that we don't have to do anything about arguments, so please ignore my confusion! 😝

@simonihmig simonihmig deleted the builtin-input branch July 23, 2019 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants