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

Upgraded components with ng-transclude error if there is no transcluded content #13271

Closed
jonrimmer opened this issue Dec 6, 2016 · 12 comments
Closed

Comments

@jonrimmer
Copy link
Contributor

jonrimmer commented Dec 6, 2016

I'm submitting a ... (check one with "x")

[x] bug report
[ ] feature request
[ ] support request

Current behavior

When an upgraded ng1 component contains an ng-transclude directive, but the element is not supplied with content to transclude — e.g. it is empty, or should use fallback content — then the following error is thrown:

VM3034 angular.js:14110TypeError: Cannot read property '$destroy' of undefined
    at ngTranscludeCloneAttachFn (https://unpkg.com/angular/angular.js:31320:29)
    at attachChildNodes (https://unpkg.com/@angular/[email protected]/bundles/upgrade-static.umd.js:568:92)
    at controllersBoundTransclude (https://unpkg.com/angular/angular.js:9542:20)
    at Object.ngTranscludePostLink (https://unpkg.com/angular/angular.js:31306:9)
    at eval (https://unpkg.com/angular/angular.js:1259:18)
    at invokeLinkFn (https://unpkg.com/angular/angular.js:10095:9)
    at nodeLinkFn (https://unpkg.com/angular/angular.js:9492:11)
    at compositeLinkFn (https://unpkg.com/angular/angular.js:8757:13)
    at MyUpgradedComponent.publicLinkFn [as linkFn] (https://unpkg.com/angular/angular.js:8637:30)
    at MyUpgradedComponent.UpgradeComponent.ngOnInit (https://unpkg.com/@angular/[email protected]/bundles/upgrade-static.umd.js:569:18)
    at Wrapper_MyUpgradedComponent.ngDoCheck (/AppModule/MyUpgradedComponent/wrapper.ngfactory.js:25:53)
    at CompiledTemplate.proxyViewClass.View_App0.detectChangesInternal (/AppModule/App/component.ngfactory.js:27:33)
    at CompiledTemplate.proxyViewClass.AppView.detectChanges (https://unpkg.com/@angular/[email protected]/bundles/core.umd.js:12522:18)
    at CompiledTemplate.proxyViewClass.DebugAppView.detectChanges (https://unpkg.com/@angular/[email protected]/bundles/core.umd.js:12669:48)
    at CompiledTemplate.proxyViewClass.View_App_Host0.detectChangesInternal (/AppModule/App/host.ngfactory.js:29:19) <ng-transclude class="ng-scope">

It seems the ng1 transclusion directive tries to destroy the transcluded scope if the transcluded content is empty, but UpgradeComponent doesn't actually supply a scope in this case.

Expected behavior

It should work without errors.

Minimal reproduction of the problem with instructions

https://plnkr.co/edit/iluIot4jXRbot4Rk3O9t?p=preview

What is the motivation / use case for changing the behavior?

Correctness, ease of upgrade.

Please tell us about your environment:

macOS Sierra

  • Angular version: 2.3.0-rc.0, 1.6.0-rc.2

  • Browser: all

  • Language: all

@jonrimmer
Copy link
Contributor Author

Shouldn't the label be upgrade/static?

@qmOnArq
Copy link

qmOnArq commented Dec 8, 2016

If you need workaround for now, you can use ng-if to remove the transclude when it's empty

@petebacondarwin
Copy link
Member

One problem that this bug has highlighted to me is that we are currently testing ngUpgrade against 1.5.x (which doesn't expose this problem) rather than 1.6.0.

@petebacondarwin
Copy link
Member

I think we need to do a better job of creating our bound transclusion function, so that it has a suitable transclusion scope passed through.

@petebacondarwin
Copy link
Member

Working on a fix for this.

@petebacondarwin
Copy link
Member

Maybe this helps: #16627

@gkalpak
Copy link
Member

gkalpak commented May 9, 2017

There are some issues with your plnkr. Here is an updated version. This still doesn't work (because of an ngUpgrade bug), but should by fixed by #16627.

@gkalpak
Copy link
Member

gkalpak commented May 9, 2017

Actually, you can see that simple transclusion works already: updated plnkr
What doesn't work (and will be fixed by #16627) is:

  • Fallback content.
  • Multi-slot transclusion.

gkalpak added a commit to petebacondarwin/angular that referenced this issue May 10, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#11044
Fixes angular#13271
gkalpak added a commit to petebacondarwin/angular that referenced this issue May 26, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

This commit only affects `upgrade/static`. The dynamic version will be fixed in
a follow-up PR.

Fixes angular#11044
Fixes angular#13271
gkalpak added a commit to petebacondarwin/angular that referenced this issue May 26, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

This commit only affects `upgrade/static`. The dynamic version will be fixed in
a follow-up PR.

Fixes angular#11044
Fixes angular#13271
gkalpak added a commit to petebacondarwin/angular that referenced this issue May 29, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

This commit only affects `upgrade/static`. The dynamic version will be fixed in
a follow-up PR.

Fixes angular#13271
gkalpak added a commit to petebacondarwin/angular that referenced this issue May 31, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

This commit only affects `upgrade/static`. The dynamic version will be fixed in
a follow-up PR.

Fixes angular#13271
gkalpak added a commit to petebacondarwin/angular that referenced this issue May 31, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#11044
Fixes angular#13271
gkalpak added a commit to petebacondarwin/angular that referenced this issue May 31, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
gkalpak added a commit to petebacondarwin/angular that referenced this issue May 31, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
gkalpak added a commit to gkalpak/angular that referenced this issue May 31, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
gkalpak pushed a commit to gkalpak/angular that referenced this issue Jul 5, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
gkalpak added a commit to gkalpak/angular that referenced this issue Jul 5, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
jasonaden pushed a commit that referenced this issue Jul 6, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes #13271
jasonaden pushed a commit to jasonaden/angular that referenced this issue Jul 6, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
jasonaden pushed a commit to jasonaden/angular that referenced this issue Jul 7, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
jasonaden pushed a commit that referenced this issue Jul 8, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes #13271
jasonaden pushed a commit to jasonaden/angular that referenced this issue Jul 8, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
jasonaden pushed a commit to jasonaden/angular that referenced this issue Jul 8, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
jasonaden pushed a commit that referenced this issue Jul 8, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes #13271
asnowwolf pushed a commit to asnowwolf/angular that referenced this issue Aug 11, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
juleskremer pushed a commit to juleskremer/angular that referenced this issue Aug 28, 2017
Previously, only simple, single-slot transclusion worked on upgraded components.
This commit fixes/adds support for the following:

- Multi-slot transclusion.
- Using fallback content when no transclusion content is provided.
- Destroy unused scope (when using fallback content).

Fixes angular#13271
@FDIM
Copy link
Contributor

FDIM commented Feb 12, 2018

I know that this is an old issue, but this seems to be still happening with angular 5.2.4 and angularjs 1.6.9:
https://stackblitz.com/edit/ngupgradestatic-playground-hh5hru

@gkalpak
Copy link
Member

gkalpak commented Feb 12, 2018

@FDIM, this issue is closed as fixed. Can you please open a new issue to track the bug you are reporting?

gkalpak added a commit to gkalpak/angular that referenced this issue Feb 12, 2018
The function provided by `ngUpgrade` as `parentBoundTranscludeFn` when
upgrading a component with transclusion, will break in AngularJS v1.5.8+
if no transclusion content is provided. The reason is that AngularJS
will try to destroy the transclusion scope (which would not be needed
any more). But since the transcluded content comes from Angular, not
AngularJS, there is no transclusion scope to destroy.
This commit fixes it by providing a dummy scope object with a no-op
`$destroy()` method.

See angular#13271 (comment).
@FDIM
Copy link
Contributor

FDIM commented Feb 12, 2018

@gkalpak done

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants