[WIP] [BUGFIX canary] Allow contextual components to be a glimmer component #12617

Serabe · 2015-11-16T18:35:56Z

Currently, contextual components does not work when the component being closured is a glimmer component. This PR aims to fix #12590

rwjblue · 2015-11-16T19:59:59Z

The path/checklist seems correct to me.

Serabe · 2015-11-20T18:27:25Z

Quick update: I haven't forgot about this. I'm doing some tests on Glimmer components before going full speed into solving the issue. I have divide first task in two since I need to implement the other feature to test it.

Serabe · 2015-11-21T20:09:08Z

GlimmmerComponents can already be rendered as contextual components. I added the task of checking which tests in closure components would make sense to have for GlimmerComponents as well. I'll be doing that next (read it as tomorrow).

I have yet to do the mut unboxing, but I think there is a simple way by adding an optional parameter to getCellOrValue. I should not be modifying any part of the render process any more (unless not significantly).

mixonic · 2015-11-22T17:13:21Z

packages/ember-htmlbars/lib/node-managers/component-node-manager.js

@@ -83,7 +85,7 @@ ComponentNodeManager.create = function ComponentNodeManager_create(renderNode, e
    if (isAngleBracket) {
      assert(`You cannot invoke the '${tagName}' component with angle brackets, because it's a subclass of Component. Please upgrade to GlimmerComponent. Alternatively, you can invoke as '{{${tagName}}}'.`, component.isGlimmerComponent);
    } else {
-      assert(`You cannot invoke the '${tagName}' component with curly braces, because it's a subclass of GlimmerComponent. Please invoke it as '<${tagName}>' instead.`, !component.isGlimmerComponent);
+      assert(`You cannot invoke the '${tagName}' component with curly braces, because it's a subclass of GlimmerComponent. Please invoke it as '<${tagName}>' instead.`, !component.isGlimmerComponent || isClosureComponent || isComponentHelper);


Is isClosureComponent threaded through here just for the assertion?

Perhaps we can just move the assertion further up the stack and avoid this. It kind of stinks to be passing these flags around.

Yes. Currently looking if I can just change isAngleBracket to mean behave like a GlimmerComponent. Otherwise, this can be hard to maintain.

Serabe · 2015-12-01T13:00:51Z

Some update on this:

I removed {auto: true} from the transformation, given that it is already being marked as internal. Furthermore, I changed slightly the code from this:

if (stream[MUTABLE_REFERENCE]) {
  return stream;
}

to:

if (stream[MUTABLE_REFERENCE] && !internal) {
  return stream;
}

given that a mutable stream needs to be marked internal to be unboxed if needed.

Some changes were introduced in the rendering process too. Some conditionals depend on the component being a GlimmerComponent, but others actually depend on it being called with angle brackets. Would it be possible to talk to someone in Slack to check a few things?

Last, I'll spend some time adding more tests, specially from #12613 to see we support GlimmerComponents properly. After all is working, I'll clean the code.

Sorry for the delay. Still working on this.

mixonic · 2015-12-01T16:14:52Z

@Serabe there has been some discussion about this lately, and it will need adjustment (sorry!). @wycats would like to hold off on invoking glimmer components with {{, which means they should not be invokable with the {{component helper or with {{dot.syntax.

However, dot syntax invocation with < should be supported. For example:

<my-form model=model as |f|>
  <f.button name='firstName' />
  <f.submit>Save</f.submit>
</my-form>

Of course this only applies to glimmer components. Happy to chat via Slack this week.

homu · 2015-12-04T18:19:55Z

☔ The latest upstream changes (presumably #12682) made this pull request unmergeable. Please resolve the merge conflicts.

Serabe · 2015-12-04T19:09:39Z

@homu, thank you. I'll be rebasing master soon, though I'll try to squash the commits so far (the conflict happens in closure_component_test.js and I hope that that is only because of 3477995, that gets mostly reverted in 9a9eab5)

HTMLBars need to be able to generate components with syntax `my.comp` for implementing contexual components. emberjs/ember.js#12617

Serabe · 2015-12-10T00:13:23Z

This PR needs tildeio/htmlbars#438 due to <my.component> being compiled to the fragment part.

rwjblue · 2015-12-11T23:09:49Z

@Serabe - Can you updated package.json and npm-shrinkwrap.json to use htmlbars@0.14.10 (manually apply the same changes as #12705 for the version you need)?

Serabe · 2015-12-12T02:13:04Z

Yes. I'll upload a PR tomorrow morning for this. I'm not uploading this tonight cause npm is taking longer than usual and I cannot check the tests 😞

Serabe · 2015-12-12T12:28:57Z

@rwjblue I have uploaded PR #12706. I cannot install the dependencies using the npm shrinkwrap file, though I can install them without it. Cannot check in a different network until Monday.

rwjblue · 2015-12-12T13:30:02Z

Thanks @Serabe!

Serabe · 2015-12-13T19:14:38Z

Adding some tests and then I'll remove the autounboxing feature. With the new requirements it is not needed.

homu · 2015-12-21T00:12:58Z

☔ The latest upstream changes (presumably #12712) made this pull request unmergeable. Please resolve the merge conflicts.

Serabe · 2015-12-21T00:35:59Z

Rebased!

homu · 2015-12-22T23:33:47Z

☔ The latest upstream changes (presumably #12746) made this pull request unmergeable. Please resolve the merge conflicts.

Example: ```hbs <my-form model=model as |f|> <f.button name='firstName' /> <f.submit>Save</f.submit> </my-form> ```

homu · 2016-03-13T04:10:40Z

☔ The latest upstream changes (presumably #13096) made this pull request unmergeable. Please resolve the merge conflicts.

rwjblue · 2016-04-10T23:46:29Z

I'm not sure of the path forward on this. We should likely let the dust settle on the glimmer integration efforts and attack again....

rwjblue · 2016-09-27T00:53:23Z

Thanks so much for working on this @Serabe, but since we did a full migration to ember-glimmer and removed support for angle bracket components I am going to close this PR.

I am sure we will work together on this again when we get angle bracket components going again...

Serabe mentioned this pull request Nov 16, 2015

attrs not updating correctly for contextual components #12613

Closed

Serabe force-pushed the feature/contextual-glimmer-components branch 2 times, most recently from f68909e to 19817f7 Compare November 22, 2015 12:40

mixonic reviewed Nov 22, 2015
View reviewed changes

Serabe force-pushed the feature/contextual-glimmer-components branch from 19817f7 to f5bf7d0 Compare December 1, 2015 12:53

Serabe force-pushed the feature/contextual-glimmer-components branch from 9a9eab5 to 6c4b264 Compare December 4, 2015 21:56

Serabe added a commit to Serabe/htmlbars that referenced this pull request Dec 10, 2015

[FEATURE] Add contextual component syntax

aca3399

HTMLBars need to be able to generate components with syntax `my.comp` for implementing contexual components. emberjs/ember.js#12617

Serabe added a commit to Serabe/htmlbars that referenced this pull request Dec 10, 2015

[FEATURE] Add contextual component syntax

f32ccae

HTMLBars need to be able to generate components with syntax `my.comp` for implementing contexual components. emberjs/ember.js#12617

Serabe mentioned this pull request Dec 10, 2015

[FEATURE] Add contextual component syntax tildeio/htmlbars#438

Merged

Serabe force-pushed the feature/contextual-glimmer-components branch from 6c4b264 to d80ac03 Compare December 11, 2015 12:49

Serabe force-pushed the feature/contextual-glimmer-components branch 2 times, most recently from a04b764 to e9c46f5 Compare December 13, 2015 21:59

Serabe changed the title ~~[WIP] [BUGFIX canary] Allow contextual components to be a glimmer component~~ [BUGFIX canary] Allow contextual components to be a glimmer component Dec 13, 2015

Serabe force-pushed the feature/contextual-glimmer-components branch from e9c46f5 to 9684049 Compare December 21, 2015 00:34

[BUGFIX canary] Implement dot syntax for contextual Glimmer components

a6e377b

Example: ```hbs <my-form model=model as |f|> <f.button name='firstName' /> <f.submit>Save</f.submit> </my-form> ```

Serabe force-pushed the feature/contextual-glimmer-components branch from 9684049 to a6e377b Compare December 23, 2015 00:09

Serabe changed the title ~~[BUGFIX canary] Allow contextual components to be a glimmer component~~ [WIP] [BUGFIX canary] Allow contextual components to be a glimmer component Apr 11, 2016

rwjblue closed this Sep 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] [BUGFIX canary] Allow contextual components to be a glimmer component #12617

[WIP] [BUGFIX canary] Allow contextual components to be a glimmer component #12617

Serabe commented Nov 16, 2015

rwjblue commented Nov 16, 2015

Serabe commented Nov 20, 2015

Serabe commented Nov 21, 2015

mixonic Nov 22, 2015

Serabe Nov 22, 2015

Serabe commented Dec 1, 2015

mixonic commented Dec 1, 2015

homu commented Dec 4, 2015

Serabe commented Dec 4, 2015

Serabe commented Dec 10, 2015

rwjblue commented Dec 11, 2015

Serabe commented Dec 12, 2015

Serabe commented Dec 12, 2015

rwjblue commented Dec 12, 2015 via email

Serabe commented Dec 13, 2015

homu commented Dec 21, 2015

Serabe commented Dec 21, 2015

homu commented Dec 22, 2015

homu commented Mar 13, 2016

rwjblue commented Apr 10, 2016

rwjblue commented Sep 27, 2016

[WIP] [BUGFIX canary] Allow contextual components to be a glimmer component #12617

[WIP] [BUGFIX canary] Allow contextual components to be a glimmer component #12617

Conversation

Serabe commented Nov 16, 2015

rwjblue commented Nov 16, 2015

Serabe commented Nov 20, 2015

Serabe commented Nov 21, 2015

mixonic Nov 22, 2015

Choose a reason for hiding this comment

Serabe Nov 22, 2015

Choose a reason for hiding this comment

Serabe commented Dec 1, 2015

mixonic commented Dec 1, 2015

homu commented Dec 4, 2015

Serabe commented Dec 4, 2015

Serabe commented Dec 10, 2015

rwjblue commented Dec 11, 2015

Serabe commented Dec 12, 2015

Serabe commented Dec 12, 2015

rwjblue commented Dec 12, 2015 via email

Serabe commented Dec 13, 2015

homu commented Dec 21, 2015

Serabe commented Dec 21, 2015

homu commented Dec 22, 2015

homu commented Mar 13, 2016

rwjblue commented Apr 10, 2016

rwjblue commented Sep 27, 2016