-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Glimmer2] Implement input #13615
[Glimmer2] Implement input #13615
Conversation
@@ -146,6 +157,11 @@ export default class Environment extends GlimmerEnvironment { | |||
assert(`A helper named '${path[0]}' could not be found`, false); | |||
} | |||
} else { | |||
let generateSyntax = builtInDynamicComponents[key]; |
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.
Seems like this should be it's own branch?
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.
Sure. It would just mean always doing the lookup up-front which seems fine.
For whatever reason, one of the input tests is failing in Safari and Microsoft browsers. I'm looking into it. |
I think saucelabs is having problems
|
ec51222
to
dc776b4
Compare
I've changed the approach to re-write the AST. Any ideas why travis doesn't seem to be running? |
wat |
} | ||
} | ||
if (pair && pair.value.type !== 'StringLiteral') { | ||
node.params.unshift(builders.sexpr('-input-type', [builders.path(pair.value.original)], null)); |
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.
Please add loc
information to these (builders.sexpr
and builders.path
).
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.
What happens if -input-type
is already specified?
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.
@rwjblue how do I add loc
information here? Neither buildSexpr
nor buildPath
accept a loc
arguments like some of the other builders. Does that need to be added?
I don't think end-users can add a -input-type
helper since it's registered as an internal helper. But if one already existed, I believe DynamicComponentSyntax
only looks at the first argument, so if -input-type
was already specified, it would be ignored.
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.
Neither buildSexpr nor buildPath accept a loc arguments like some of the other builders. Does that need to be added?
Yes. Each AST node should be able to be built with a loc.
I don't think end-users can add a -input-type helper since it's registered as an internal helper.
There is nothing stopping a user from having already defined a -input-type
helper in their application, but I'm not sure we should care about that too much...
I believe DynamicComponentSyntax only looks at the first argument, so if -input-type was already specified, it would be ignored.
If we expect this to go into exactly node.params[0]
we should do that, and error if there was already something there.
For example today this "works" (doesn't blow up):
{{input "blah blah blah" value=someThing}}
If we are going to add (internally) significance to the first ordered argument to {{input
, then we should have a helpful assertion if the user puts something there...
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.
@rwjblue I meant that the end-user cannot use -input-helper
unless they themselves define -input-helper
. I confirmed this locally. My understanding is that a leading dash is reserved for internal helpers.
EDIT: In other words, the -input-helper
I am introducing is not available for end users. They would have to make their own, which I'm under the impression is not supposed to happen given the convention.
I can certainly add a check to blow up if there's any pre-existing positional params. In other words, we are now saying that you cannot use positional parameters with {{input}}
.
Looking good! |
Working on figuring out why tests fail in Safari and MS browsers. Safari is failing for a different reason than MS. |
Modifying input.selectionStart, which is utilized in the cursor tests, causes an event in Safari (does not happen in Chrome). This event kicks off a chain of events that results in an error because the view does not have |
@asakusuma Yea we need to land #13532. |
☔ The latest upstream changes (presumably #13492) made this pull request unmergeable. Please resolve the merge conflicts. |
@asakusuma Should you skip the failing test for now and we can recheck after sendAction? |
I'm fine doing that, though I'm not sure All the tests pass in Chrome, Safari, and Firefox. In Microsoft browsers, the null values test fails. What's failing is that when you set the value of the input to |
☔ The latest upstream changes (presumably #13684) made this pull request unmergeable. Please resolve the merge conflicts. |
Discovered the source of the problem in MS Edge. Here's a simple repro: https://gist.github.com/asakusuma/aad85f430f465903e56feee2b53a414c cc @jdalton |
@asakusuma - Have you reported an issue against MS Edge? |
@jdalton in MS Edge, I have not yet filed a ticket. |
☔ The latest upstream changes (presumably #13649) made this pull request unmergeable. Please resolve the merge conflicts. |
15e576f
to
05f25dd
Compare
@asakusuma I filed a ticket internally. |
@jdalton - Thank you! |
I've gotten |
@jdalton thanks! I'm not aware of any other problem attributes. |
Just a heads up, while making a feature detection for this I noticed that in Chrome: var input = document.createElement('input');
var buggyRemoveAttrValue = !!(input.value=1,input.removeAttribute('value'),input.value);
buggyRemoveAttrValue;
// => true in Chrome and Edge but var div = document.createElement('div');
div.innerHTML = '<input value=1>';
var input = div.firstChild;
var buggyRemoveAttrValue = !!(input.removeAttribute('value'),input.getAttribute('value'));
buggyRemoveAttrValue;
// => false in Chrome but true in Edge It's the old source attribute vs. element property thing. |
☔ The latest upstream changes (presumably #13725) made this pull request unmergeable. Please resolve the merge conflicts. |
Did some bisecting, and it would appear that glimmerjs/glimmer-vm@951cbe5 introduced an incompatibility with the ember build.
|
@asakusuma I think this is related to glimmerjs/glimmer-vm#190, we need to ship |
After including the glimmer fix, Travis is now failing with a new problem
|
@rwjblue do we need to increase the timeout? Blueprints is also failing but I'm not seeing a useful error.
|
npm is still borking on the |
Cleared cache and restarted builds. |
Thank you @asakusuma for sticking with this so long! |
I don't think the late bound stuff can help with implementing
input
sincelateBound
gets called afterCurlyComponentManager.create
.cc @krisselden @chadhietala @GavinJoyce