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.

Sign up for GitHub

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

Jump to bottom

Unmount angular by using the proper rootScope #24

Merged
merged 4 commits into from
Sep 6, 2016
Merged

Unmount angular by using the proper rootScope #24

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
363b546
Unmount angular by using the proper rootScope
blittle Sep 6, 2016
7000a30
Make it work in prod mode
blittle Sep 6, 2016
a4778d3
Use an ID to make things more flexible
blittle Sep 6, 2016
0e18466
Make element id configurable
blittle Sep 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/single-spa-angular1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ function mount() {

function unmount() {
return new Promise((resolve, reject) => {
let rootScope = angular.injector(['ng']).get('$rootScope');
let rootElement = angular.element(getContainerEl().childNodes[0]);
Copy link
Contributor Author

@blittle blittle Sep 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This is querying the first element that angular mounted which should have the $rootScope.

Copy link

Choose a reason for hiding this comment

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

Only concern is for this to throw an uncaught error

Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that the first childNode is the correct one?

Copy link
Member

Choose a reason for hiding this comment

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

Could we do something like getContainerEl().querySelector('[ng-scope]')

Copy link
Member

Choose a reason for hiding this comment

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

And if so, does angular put that ng-scope attribute on both in debug mode and production mode??

Copy link
Contributor Author

@blittle blittle Sep 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@joeldenning There is no ng-scope attribute :( Maybe because I am debugging in prod!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kentmclean if it does throw an error, what should the outcome be? My thoughts are if there is an error, it is a catastrophic error. Is it something we could ignore? Maybe if we mount the app and unmount it super fast (before the dom has been built) then the [0] could be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joeldenning there is a className present with ng-scope

Copy link

Choose a reason for hiding this comment

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

@blittle - an error here would be unexpected for sure, probably due to a timing issue or an unintended re-unloading of an app. For our customers I'd want the error eaten as gracefully as possible. Then for our own purposes (and I assume this is already happening) we want to have the error alerted on through Sentry with relevant autoTrace stack.

let rootScope = rootElement.scope();

const result = rootScope.$destroy();

getContainerEl().innerHTML = '';
Expand Down