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

[Glimmer2] [CLEANUP canary] Remove deprecated legacy-each support including {{collection}} #13161

Merged
merged 1 commit into from
Mar 24, 2016

Conversation

HeroicEric
Copy link
Sponsor Member

I spent some time trying to figure out how to remove the collection helper/keyword/view for #13127. As far as I can tell, a bunch of other things depend on it and also need to be removed in order to do so.

  • {{each itemView=}}
  • {{each itemViewClass=}}
  • {{each itemController=}}
  • {{each tagName=}}
  • {{each emptyView=}}
  • {{each emptyViewClass=}}
  • {{collection}}

Much of my thoughts here are based on my understanding of what's going on in https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-each-into-collection.js#L43-L57

There is still cleanup to do here but I tried to organize the commits so that they could possibly be brought in separately. That worked fine until I got to actually removing the

Questions

  1. Am I correct in thinking that these things also need to be removed?
  2. Should I break out these commits into smaller PRs?

Guidance welcomed 😄

@@ -1,21 +1,9 @@
{{~#each view._arrangedContent -legacy-keyword=view.keyword as |item|~}}
Copy link
Member

Choose a reason for hiding this comment

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

I think this whole file should 🔥

@rwjblue
Copy link
Member

rwjblue commented Mar 22, 2016

Am I correct in thinking that these things also need to be removed?

Yes

Should I break out these commits into smaller PRs?

No

@HeroicEric HeroicEric force-pushed the remove-collection branch 2 times, most recently from a597a00 to 7ab839f Compare March 23, 2016 00:01
@HeroicEric HeroicEric changed the title [WIP] Remove collection test Remove deprecated legacy-each support including {{collection}} Mar 23, 2016
@HeroicEric
Copy link
Sponsor Member Author

@rwjblue I still need to update the two references to CollectionView in the examples I tagged in the last commit but I think the rest is ready to go.

Should squash when done?

registerPlugin('ast', AssertNoViewAndControllerPaths);
registerPlugin('ast', AssertNoViewHelper);
}
registerPlugin('ast', AssertNoViewAndControllerPaths);
Copy link
Member

Choose a reason for hiding this comment

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

These should only be registered if the legacy view flag is off. We definitely should have a follow up PR to remove the flag and all usages of it, but it should be a separate change from this one IMO.

@rwjblue
Copy link
Member

rwjblue commented Mar 23, 2016

I left one more tweak, but this is looking good. Once you have those docs updated, go ahead and squash and we can land this...

For emberjs#13127

- Remove {{each itemView=}}
- Remove {{each itemViewClass=}}
- Remove {{each itemController=}}
- Remove {{each tagName=}}
- Remove legacy-each-*
- Remove TransformEachIntoCollection
- Remove `CollectionView`

This was done in order to clean up deprecated features to make the
transition to Glimmer 2 easier, since these features won't need to be
supported by Glimmer 2.
@HeroicEric HeroicEric changed the title Remove deprecated legacy-each support including {{collection}} [CLEANUP canary] Remove deprecated legacy-each support including {{collection}} Mar 23, 2016
@HeroicEric HeroicEric changed the title [CLEANUP canary] Remove deprecated legacy-each support including {{collection}} [Glimmer2] [CLEANUP canary] Remove deprecated legacy-each support including {{collection}} Mar 23, 2016
@HeroicEric
Copy link
Sponsor Member Author

I think this is all set 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥

@@ -114,13 +114,16 @@ NativeArray = NativeArray.without.apply(NativeArray, ignore);

Example

TODO: Update example to not use CollectionView
Copy link
Member

Choose a reason for hiding this comment

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

Need to kill this TODO comment, looks like the docs below already took care of it...

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in b295edb.

@rwjblue
Copy link
Member

rwjblue commented Mar 24, 2016

Sorry, just one more nit to pick...

@rwjblue rwjblue merged commit 2b2fa57 into emberjs:master Mar 24, 2016
@rwjblue
Copy link
Member

rwjblue commented Mar 24, 2016

Thanks for this @HeroicEric!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants