Glimmer component #12011
Merged
Glimmer component #12011
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
Previously, Glimmer components got their attrs set after positional attrs were applied. The `didInitAttrs` hook was used to indicate that the full set of attributes were applied. Recently, positional attrs were changed to ensure that the full set of attrs are always applied before `init`, eliminating the need for `didInitAttrs`. However, the existing code that applied attrs on init only did so for non-angle bracket components (to avoid double-setting on new-world features). Consequently, angle bracket components *never* got their attrs set. This commit ensures that `attrs` are set on all components before init, regardless of style.
1. {{my-component ...}} curly braces can only be used on legacy Component
2. <my-component ...> angle brackets can only be used on GlimmerComponent
TBD: rule number 2 might be loosened with an explicit opt-in later,
depending on how painful this transition turns out to be for large apps.
It might be better to provide automated rewriting through ember-watson,
which would do the job quickly, once and for all, rather than leaving
large apps in a semantic limbo with one foot in the old world and
another foot in the new world.
They were disabled due to a bug in the feature flag infrastructure; but it has since been fixed and everything appears to be working again here.
This code was previously telling HTMLBars to post-process content templates to add top-level attributes. That is not correct (it only makes sense to post-process layouts).
If the layout for `<my-component>` has `<my-component>` as its root element, this is a special case. The semantics of this case are intended to be the same as a top-level `<div>`. However, in an attempt to share code, we inadvertantly introduced a bug where we installed the dynamic attributes defined by JS APIs twice (via `normalizeComponentAttributes` and post-processing of the parent template). The fix, therefore, is to not do anything special with dynamic attributes and rely on the same post-processing step as the `<div>` case.
| @@ -19,7 +19,13 @@ export default function bindSelf(env, scope, _self) { | |||
| if (self && self.isView) { | |||
| newStream(scope.locals, 'view', self, null); | |||
| newStream(scope.locals, 'controller', scope.locals.view.getKey('controller')); | |||
| newStream(scope, 'self', scope.locals.view.getKey('context'), null, true); | |||
|
|
|||
| if (self.isGlimmerComponent) { | |||
There was a problem hiding this comment.
should we instead use polymorphism here? Something like this._setupSelfStream() and let the subclasses refine further?
|
:D |
added 5 commits
August 7, 2015 15:06
This was previously used for attaching outer attributes as well as system attributes (e.g. class="ember-view" id="ember123") onto regular HTML elements at the root, once they have been rendered. This results in multiple AttrNodes for a single attribute, which means that there is no guarantee that any particular AttrNode will actually "win". This incorrect strategy "works" a surprising amount of the time, but the failure modes are unacceptable. (See new tests introduced in the previous commit.) This is in anticipation of making all top-level elements in a component's layout dynamic, so that they can share attribute merging logic with the "identity element" case (top-level `<my-component>` in the layout for `my-component`).
This was annoying because the ordering of non-user attributes are unimportant, but can change (and has changed) depending on the implementation.
Also re-enabling a test that was actually fixed in the previous commit.
See "non-block ... should have correct scope" test cases
|
We should address the plan for (or intentional avoidance) of the nested component case (components in sub-dirs) #10456. |
|
@rwjblue seems legit. I assume you mean via documentation, right? |
|
@wycats - Yeah, we should have docs and tests that say exactly what we do and don't support subdirectory wise. I'm not suggesting that we have to support subdirs for landing + enabling, but I'd like to be very clear about what we intend (and that we didn't "forget"). |
added 3 commits
August 9, 2015 18:47
To reduce the possibility of regressions when the `ember-htmlbars-component-generation` flag is off, this commit moves an important AST transformation behind the flag. This AST transformation makes top-level elements in *all* templates dynamic, and requires runtime work to restore the original semantics. Moving the AST transformation behind the flag limits the impact of any bugs in the runtime work.
In Ember 1.x components, the top-level element was configured in
JavaScript using APIs like `classNames`, `attributeBindings`, `tagName`
etc.
Glimmer components move that configuration into a top-level element in
the template. However, the template compiler does not know how the
template will actually be used, so it makes all top-level elements
dynamic, and leaves it up to the runtime to figure out what to do.
If the template is being invoked for a curly component, the runtime
restores the original semantics, treating it just like a regular
element.
If the template is being invoked for an angle-bracket component, the
top-level element is treated as the component’s element.
This commit makes top-level `<div>`s work correctly in Glimmer
components (they become the component’s element), and work correctly
in curly components (they behave as before).
Both modifiers (`<div {{action “foo”}}>`) and triple-curly attributes
are not supported in top-level elements of Glimmer components. As a
result, if the AST transformation sees either of those two features,
it assumes the template is for a curly component, and does not make the
root element dynamic.
At the moment, it would be impossible to support either of those two
features, since the component AST node in HTMLBars do not support them.
Ultimately, we may want to support triple-curly attributes, but probably
not modifiers.
Note that this commit requires an update to HTMLBars master.
added 8 commits
August 10, 2015 14:17
…nt_computed_function Failing test for computed property alias in angle-bracket component
Don’t bother to convert top-level elements into dynamic calls if they
aren’t the top-level element *of a template*.
Consider a template like this:
```
{{#each posts as |post|}}
<div>hello</div>
{{/each}}
```
The previous version of the transform would convert the nested template
(the <div>) into a dynamic call, because the AST transformation didn’t
differentiate between top-level templates and nested templates. It
didn’t have any other effects (the dynamic call is required to restore
the semantics of the original static form) but it unnecessarily bloats
templates and makes execution slower.
There are multiple competing proposals for how to represent fragments in GlimmerComponents, and we have not yet settled on the exact solution. In the interim, GlimmerComponents require a top-level wrapper element. This commit also provides a good error message describing exactly what the user did wrong. In the future, when fragments are allowed, we can update the error message to suggest opting into a fragment.
This feature works in all browsers in our support matrix except for PhantomJS, so the Travis tests are failing.
The current ember-debug transformations do not support nested asserts inside runInDebug.
As of the previous commit, any use of `<foo-bar>` was assumed to be an Ember Glimmer Component. This breaks code that was using dasherized names either to refer to web components or just to produce regular HTML elements. We believe that both of these are done in practice. This commit allows `<foo-bar>` to be treated the same as `<div>` in all currently implemented contexts.
Previously, we had hardcoded tests for two styles of top-level element in a GlimmerComponent. For the layout of `<foo-bar>`, we had tests for the top-level element being `<foo-bar>` itself and being a `<div>`. A recent commit allowed the top-level element to also be any dasherized element that was not registered as an Ember component (aka “web components”). This commit modifies the tests to enumerate all three styles and runs the same set of tests for each.
|
We also need to double check the GlimmerComponent class is exported correctly (right module path and globally mode too?) |
|
Probably needs an RFC, but it would be great if glimmer components could optionally specified required types of attrs during dev. It would make integrating components from addons a much more pleasant experience, might also have perf wins in prod due to the fact that the code was designed to be called consistently with the same object types. This would be like |
added 2 commits
August 13, 2015 10:31
Currently a top-level partial that happens to have a single top-level element confuses the heuristics in the component hook.
|
@chadhietala +1 on that, I try to write helpful asserts but also hate writing them all the time. |
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.
Don't merge this yet, until the following items are complete:
<my-foo>top level inside<my-foo>) - done in d280c53component.elementmay not be set onGlimmerComponentfa01e24<input value={{bar}} />){{#with}}<div {{action}}></div>{{/with}}works (6424797)GlimmerComponent(5b686d4){{action "foo"}}orstyle={{{bar}}}Post-merge:
{{partial}}case (0604db1)tagName)<my-foo>component as the top-level element inside<my-bar>'s layout)