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

Glimmer component #12011

Merged
merged 34 commits into from Aug 17, 2015

Conversation

Projects
None yet
8 participants
@wycats
Member

wycats commented Aug 7, 2015

Don't merge this yet, until the following items are complete:

  • reintroduce mergeAttrs & normalization for identity elements (<my-foo> top level inside <my-foo>) - done in d280c53
  • remove attributes hook (post-processing for top-level elements) - done in d280c53
  • figure out why component.element may not be set on GlimmerComponent fa01e24
  • identify and fix scope bug in identity elements c745c47
  • make regular elements like identity elements -- done in ca436a2 and 87e021a
  • align fallback semantics with htmlbars (<input value={{bar}} />)
  • Re-enable previously skipped tests related to angle bracket components.
  • Fix and incorporate currently failing test from #11991 (test added in 6f359d2, fixed previously in c745c47)
  • Make sure {{#with}}<div {{action}}></div>{{/with}} works (6424797)
  • Error when you use a fragment inside GlimmerComponent (5b686d4)
    • Better error when fragment comes from {{action "foo"}} or style={{{bar}}}
  • Make sure existing uses of already-registered web components continue to work (974493f)
  • all green??????????

Post-merge:

  • Fix top-level {{partial}} case (0604db1)
  • Make sure that people don't accidentally use old-style APIs for top-level elements in GlimmerComponent (tagName)
  • Refactor to separate conceptually different code paths that happen to share code, such as...
    • build-component-template
    • component node manager?
  • make recursive invocation work (<my-foo> component as the top-level element inside <my-bar>'s layout)
  • discuss a path for properties vs. attributes being plausibly aligned between normal elements and components

funtusov and others added some commits Aug 5, 2015

Godhuda
Fix tests
1. Make sure tests that test Glimmer Components are feature flagged.
2. Update a test to use `init` instead of `didInitAttr` due to a change to
    the lifecycle that landed on master.
Godhuda
Fix attr setting in Glimmer Components
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.
Godhuda
Show error for component class/invocation mismatch
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.
Godhuda
Re-enable skipped tests for GlimmerComponents
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.
Godhuda
Remove incorrect attributes
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).
Godhuda
This commit fixes a top-level element bug
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) {

This comment has been minimized.

@stefanpenner

stefanpenner Aug 7, 2015

Member

should we instead use polymorphism here? Something like this._setupSelfStream() and let the subclasses refine further?

@stefanpenner

stefanpenner Aug 7, 2015

Member

should we instead use polymorphism here? Something like this._setupSelfStream() and let the subclasses refine further?

@runspired

This comment has been minimized.

Show comment
Hide comment
@runspired

runspired Aug 7, 2015

Contributor

:D

Contributor

runspired commented Aug 7, 2015

:D

Godhuda added some commits Aug 7, 2015

Godhuda
Remove post-processing ("attributes") hook
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`).
Godhuda
Refactor test cases to not test exact output
This was annoying because the ordering of non-user attributes are
unimportant, but can change (and has changed) depending on the
implementation.
Godhuda
Skip failing tests for now
Also re-enabling a test that was actually fixed in the previous commit.
Godhuda
Fix GlimmerComponent scoping issue
See "non-block ... should have correct scope" test cases
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode
Member

chancancode commented Aug 8, 2015

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Aug 10, 2015

Member

We should address the plan for (or intentional avoidance) of the nested component case (components in sub-dirs) #10456.

Member

rwjblue commented Aug 10, 2015

We should address the plan for (or intentional avoidance) of the nested component case (components in sub-dirs) #10456.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Aug 10, 2015

Member

@rwjblue seems legit. I assume you mean via documentation, right?

Member

wycats commented Aug 10, 2015

@rwjblue seems legit. I assume you mean via documentation, right?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Aug 10, 2015

Member

@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").

Member

rwjblue commented Aug 10, 2015

@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").

Godhuda added some commits Aug 10, 2015

Godhuda
Skip AST transform without Glimmer Components
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.
Godhuda
Make regular elements like identity elements
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.

Godhuda added some commits Aug 10, 2015

Godhuda
Merge pull request #11991 from funtusov/failing_angle_bracket_compone…
…nt_computed_function

Failing test for computed property alias in angle-bracket component
Godhuda
Optimize the AST transform for top-level elements
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.
Godhuda
Disallow fragments in GlimmerComponent for now
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.
Godhuda
Implement nextElementSibling in JS
This feature works in all browsers in our support matrix except for
PhantomJS, so the Travis tests are failing.
Godhuda
Reorganize for transformation bug
The current ember-debug transformations do not support nested asserts
inside runInDebug.
Godhuda
Make non-Ember-component <foo-bar> work
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.
Godhuda
Test all styles of top-level element in <GC>
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.
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Aug 12, 2015

Member

We also need to double check the GlimmerComponent class is exported correctly (right module path and globally mode too?)

Member

chancancode commented Aug 12, 2015

We also need to double check the GlimmerComponent class is exported correctly (right module path and globally mode too?)

@chadhietala

This comment has been minimized.

Show comment
Hide comment
@chadhietala

chadhietala Aug 13, 2015

Member

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 propTypes in react.

Member

chadhietala commented Aug 13, 2015

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 propTypes in react.

Godhuda added some commits Aug 13, 2015

Godhuda
Add failing test for top-level {{partial}}
Currently a top-level partial that happens to have a single top-level
element confuses the heuristics in the component hook.
@runspired

This comment has been minimized.

Show comment
Hide comment
@runspired

runspired Aug 13, 2015

Contributor

@chadhietala +1 on that, I try to write helpful asserts but also hate writing them all the time.

Contributor

runspired commented Aug 13, 2015

@chadhietala +1 on that, I try to write helpful asserts but also hate writing them all the time.

rwjblue added some commits Aug 16, 2015

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Aug 16, 2015

Member

Thanks :D

Member

chancancode commented on c4e43a2 Aug 16, 2015

Thanks :D

rwjblue added a commit that referenced this pull request Aug 17, 2015

@rwjblue rwjblue merged commit ae6c9e8 into master Aug 17, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rwjblue rwjblue deleted the glimmer-component branch Aug 17, 2015

@rwjblue rwjblue referenced this pull request Aug 17, 2015

Closed

Glimmer Components Checklist #12129

0 of 12 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment